[Libreoffice] [REVIEW] [PUSHED] fdo#38457: Crash in DataPilot on moving the fields
kmachalkova at suse.cz
Thu Jun 30 05:42:55 PDT 2011
> I don't have an environment here where I can verify this, but somehow
> this fix makes me a bit nervous.
> The last time I checked, in the old code bForFile could still be true.
Nope, it's never set to true anymore (checked very recent 3.3.1 sources). And
afaict (after doing some intensive code archaeology), it's been like that for
~7 years now.
> I also assumed that it was always false and removed the if statement,
> but that caused a crasher bug (because it could be true sometimes).
Commit 523a8f41388f6d looked like this:
- SCCOL nColAdd = 0;
- if ( bForFile )
- // in old file format, columns are within document, not within source
- DBG_ASSERT( pSheetDesc, "FillOldParam: bForFile, !pSheetDesc" );
- nColAdd = pSheetDesc->GetSourceRange().aStart.Col();
+ SCCOL nColAdd = pSheetDesc->GetSourceRange().aStart.Col();
ie. it removed 'if' statement and replaced it with unconditional querying the
sheet description. The crasher you mention was then caused by the fact that
pSheetDesc can be NULL sometimes, more specifically when external db is used
(commit 45f9a8b3e2f1f4c fixes that)
> Have you guys checked a scenario where the source data range begins
> with a column other than Column A? That's when the nSrcColOffset can
> become non-zero, and setting it to always zero *might* break it
> especially for that scenario.
I'm not really sure if I get the point here, but I guess this doc from test
files repo: http://cgit.freedesktop.org/libreoffice/contrib/test-
is an example of such scenario. Test data range begins at sheet1, row3, col2.
It used to crash on field move before, doesn't crash anymore now.
\\\\\ Katarina Machalkova
\\\\\\\__o LibO developer
__\\\\\\\'/_ & hedgehog painter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 198 bytes
Desc: This is a digitally signed message part.
More information about the LibreOffice