Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

Julien, 

Looks to me like a very nice catch ... 

Here is the version as of 2011.06.15 showing braces on the for loops in both
then and else blocks...
from open grok  -- 
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=1b363f632110e80ead67ff376e92e4487556ca55

   3735 
   3736         if (bSize)
   3737         {
   3738             SCCOLROW nCols[2] = { nStartCol, nEndCol };
   3739             SCCOLROW nRows[2] = { nStartRow, nEndRow };
   3740 
   3741             for (SCTAB nTab=0; nTab<nTabCount; nTab++)
   3742                 if (aMark.GetTableSelect(nTab))
   3743                 {
   3744                     SetWidthOrHeight( sal_True, 1,nCols, nTab,
SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
   3745                     SetWidthOrHeight( false,1,nRows, nTab,
SC_SIZE_VISOPT, 0, false, false);
   3746                     rDocShell.PostPaint( 0,0,nTab,
MAXCOL,MAXROW,nTab,
   3747                                     PAINT_GRID | PAINT_LEFT |
PAINT_TOP );
   3748                 }
   3749         }
   3750         else
   3751         {
   3752             for (SCTAB nTab=0; nTab&lt;nTabCount; nTab++)
   3753                 if (aMark.GetTableSelect(nTab))
   3754                 {
   3755                     sal_Bool bAdj = AdjustRowHeight(
ScRange(nStartCol, nStartRow, nTab,
   3756                                                         nEndCol,
nEndRow, nTab), false );
   3757                     if (bAdj)
   3758                         rDocShell.PostPaint( 0,nStartRow,nTab,
MAXCOL,MAXROW,nTab,
   3759                                             PAINT_GRID | PAINT_LEFT
);
   3760                     else
   3761                         rDocShell.PostPaint( nStartCol, nStartRow,
nTab,
   3762                                             nEndCol, nEndRow, nTab,
PAINT_GRID );
   3763                 }
   3764         }

The next version (2011.07.05) 
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=2b88f6d32f572792597ccbb15276b9db52db7d10
is a commit labelled &quot;change remaining manual loops to
ScMarkData::iterator &quot; and the braces disappear on just one of the
loops.  Because the braces were once there and, especially because the else
block has ~ same action within the braces, I say that they should definitely
come back.  Even though, I am not at all clear what this code is doing
(whilst autoformatting selected cells in Calc)... this is an obvious oopsie
within a greater code cleanup that should be corrected.  

This is not fdo# 47466 --  that crash gets close but doesn't quite get to
this code: that non-reproducible crash occurred inside the AutoFormat
function called just above the missing braces.

After some more looking at the code, it appears to be applying widths and
heights... Some testing shows that if one selects a tabular area and apply a
non-default width and height (Format->Rows/Columns->Height/Width) then and
uncheck AutoFormat Width and Height under AutoFormat... -> More.  Doing the
AutoFormat then forgets the width (doesn't loop) but not the height (still
loops).  

The block of code in question starts to look like maybe it is fix code that
corrects an overactive AutoFormat function called just above.  

This here bug  --  https://bugs.freedesktop.org/show_bug.cgi?id=51524
Shows similar loss-of-width behavior in collapse/uncollapse (as opposed to
straight up AutoFormat)

There are also several reports of loss of height information (including
self-healing upon edit) - most of them are closed &/or related to file
open/close.  Maybe this block of W/H fix code is missing elsewhere, maybe it
is not needed here anymore, maybe the open & save fix the values better than
this now, maybe this simply is the width/height portion of the AutoFormat
... hmmmm 
 
I will check to see if inserting braces in the code fixes whatever
loss-of-width/height behavior that I can  reproduce or find.  

My 2cents... and now a couple more: 

I note that, for all its type mushiness, BASIC will not allow this kind of
mistake.  The then-clause must either be all on one line (can do IF test
THEN StmtA : StmtB : StmtC) and a multi-line then-clause must be terminated
with End If or Else. 

Julien, I think you should get the patch credit - this is your find.  
However, if it will help you continue to run cppchecks and find more of
these interesting puzzles, I would be happy to patch a pair of braces for
you and mark the appropriate bugs as resolved by you ;-D
 
Sweet catch, 
LeMoyne





--
View this message in context: 
http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726p4026751.html
Sent from the Dev mailing list archive at Nabble.com.