[Libreoffice] [REVIEW] [PUSHED] fdo#38457: Crash in DataPilot on moving the fields

[Katarina Machalkova]

Hey guyzz,

> 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 
range
-
-        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-
files/tree/calc/data-pilot/referenced-field-custom-name.ods
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.

B.
-- 
  \\\\\              Katarina Machalkova    
  \\\\\\\__o          LibO developer
__\\\\\\\'/_          & hedgehog painter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110630/9d668638/attachment.pgp>