Re: Questions regarding code peculiarities

On 30/06/12 23:37, Philipp Riemer wrote:

> => sw/source/core/attr/calbck.cxx
> (2) The two functions "SwClient* SwClientIter::First" and "SwClient*
> SwClientIter::Last" seem to differ in their logic. Same seems to be
> true for "SwClientIter::Next" and "SwClientIter::Previous". Or is this
> marginal change in each case correct? Can please anyone more
> experienced than me take a quick look at those functions?

my experience tells me that SwClient is completely crazy and that
changing it in any way probably breaks stuff.

> => sw/source/core/attr/cellatr.cxx
> (3) The switch statement in "SwTblBoxFormula::ChangeState" has no
> default case. This might be helpful to capture bogus behaviour...

hmm... it depends on whether it currently handles all possible values,
or only some values.  sadly i don't know what it does...

> (4) The function "SwTblBoxFormula::Calc" has only an "if" statement
> with no "else" case and also no other code outside. Is this intended?
> If so, wouldn't it be more readable if the logic is reversed, e.g.
> "if( rCalcPara.rCalc.IsCalcError() ) return; //explanation why", and
> everything else is written without being in an "if" block?

yes, i tend to prefer "early returns", because it makes it immediately
obvious that the method is not going to do anything any more in this
case.  ok, that is not so important in this 10 line function, but when
the function is 500 lines long it helps.

> => sw/source/core/attr/fmtwrapinfluenceonobjpos.cxx
> (5) The switch statement in "SwFmtWrapInfluenceOnObjPos::QueryValue"
> has only one case and thus can be replaced by an if/else; same for
> switch statement in "SwFmtWrapInfluenceOnObjPos::PutValue".
> See <https://gerrit.libreoffice.org/#/c/251/>.

hmmm why not...

> => sw/source/core/attr/format.cxx
> (7) Switch case 0 in "SwFmt::Modify" seems to be strange since it
> seems to be unexpected (from the comment) but returns directly without
> logging an error or the like...

yeah, looks like it wants to be an assert()...