Re: [Libreoffice] [PATCH] some refactoring in xmlcelli.cxx

Hello Eike,

2011/8/27 Eike Rathke <ooo@erack.de>

> Hi Markus,
>
> On Saturday, 2011-08-27 15:24:31 +0200, Markus Mohrhard wrote:
>
> > -                    if (     (!pOUTextContent && !pOUText &&
> !pOUTextValue)
> > -                        && ( (pOUTextContent &&
> !pOUTextContent->getLength()) || !pOUTextContent )
> > -                        && ( (pOUText && !pOUText->getLength()) ||
> !pOUText )
> > -                        && ( (pOUTextValue &&
> !pOUTextValue->getLength()) || !pOUTextValue ))
> > +                    if (!pOUTextContent && !pOUText && !pOUTextValue)
>
> Might it be that the original intention was
>
>                       if (     (!pOUTextContent && !pOUText &&
> !pOUTextValue)
>                           || ( (pOUTextContent &&
> !pOUTextContent->getLength()) || !pOUTextContent )
>                           || ( (pOUText && !pOUText->getLength()) ||
> !pOUText )
>                           || ( (pOUTextValue && !pOUTextValue->getLength())
> || !pOUTextValue ))
>
> instead? Just from looking at it, without having seen the context
> around..

No that makes even less sense. The only other solution that would make sense
if we were not using boost::optional would be:(I added a bit more space to
make the change clear)

                    if (     (!pOUTextContent && !pOUText && !pOUTextValue)
                       &&     (      ( (pOUTextContent &&
!pOUTextContent->getLength()) || !pOUTextContent )
                       && ( (pOUText && !pOUText->getLength()) || !pOUText )
                       && ( (pOUTextValue && !pOUTextValue->getLength()) ||
!pOUTextValue ))         )

But even this makes the first line obsolete and does not fit into the
context here. We use boost::optional and for empty values we don't use empty
strings but call boost::optional::reset() (accordingly to the boost
documentation this method is deprecated and should be replaced). And I think
if this would have been the right solution our cell import would have been
broken for at least two years without any notice. This check should check
for empty cells and skipt the import for them so I think that my first idea
is correct. Maybe this is the result of some old code that did not use
boost::optional but used empty strings for empty values.

Regards,
Markus