Re: [Libreoffice-commits] core.git: COLFUZZY SwWriteTableCol

CppunitTest_sw_ooxmlexport started to fail after 
<http://cgit.freedesktop.org/libreoffice/core/commit/?id=8f7c127c4be32a91052ea962aafd0af8a1645714> 
"Fix for LibreOffice crash during Document RoundTrip" though it is 
unclear to me whether it is due to that commit's change to 
sw/source/filter/writer/wrtswtbl.cxx or whether it is a problem that has 
been in the code before and only now happens to be triggered by that 
commit's newly added test in sw/qa/extras/ooxmlexport/ooxmlexport.cxx.

My --enable-dbgutil build started to abort in 
CppunitTest_sw_ooxmlexport, complaining about unsorted input to 
std::lower/upper_bound in o3tl/sorted_vector.hxx, which I band-aided 
with the below commit.  (Though that left CppunitTest_sw_ooxmlexport 
still failing with "number of nodes is incorrect" test failures.)

However, the code in sw/source/filter/inc/wrtswtbl.hxx leaves me 
completely puzzled.

* For one,

> inline int SwWriteTableCol::operator<( const SwWriteTableCol& rCol ) const
> {
>     // Da wir hier nur die Wahrheits-Grade 0 und 1 kennen, lassen wir lieber
>     // auch nicht zu, dass x==y und x<y gleichzeitig gilt ;-)
>     return nPos < rCol.nPos - COLFUZZY;
> }

is clearly broken and even does not fulfill the "fuzzy compare" of 
treating Cols with |a.nPos - b.nPos| <= COLFUZZY as equivalent, as 
codified in the accompanying

> inline int SwWriteTableCol::operator==( const SwWriteTableCol& rCol ) const
> {
>     // etwas Unschaerfe zulassen
>     return (nPos >= rCol.nPos ? nPos - rCol.nPos
>                                      : rCol.nPos - nPos ) <= COLFUZZY;
> }

(Note esp. that nPos is sal_uInt32, so e.g. "1000 < 10 - COLFUZZY" is true.)

* For another, I /guess/ that what is intended here is that 
SwWriteTable.aCols "normalizes" the given input and does not insert a 
new Col whose nPos is <= COLFUZZY apart from any existing Col.  But the 
code clearly fails to deliver that...

* Also, SwXMLTableColumn_Impl inherits the broken 
SwWriteTableCol::operator< and is used in a similarly dubious way in 
sw/source/filter/xml/xmltble.cxx.

Does anybody have insight into this part of the code and can properly 
fix the mess?

Stephan


On 11/15/2013 12:08 PM, Stephan Bergmann wrote:
> commit a3886c529cefdc0cb3db8ca27b3ea02f456c0470
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Fri Nov 15 12:06:34 2013 +0100
>
>      o3tl::sorted_vector Compare needs to be a strict weak ordering
>
>      No idea what the odd COLFUZZY SwWriteTableCol::operator == and < are supposed to
>      be good for, so leave them alone for now.
>
>      Change-Id: I52528a097f18ff12fac9725d802a3988c9dfa7e2
>
> diff --git a/sw/source/filter/inc/wrtswtbl.hxx b/sw/source/filter/inc/wrtswtbl.hxx
> index f4b7d50..6c25b8a 100644
> --- a/sw/source/filter/inc/wrtswtbl.hxx
> +++ b/sw/source/filter/inc/wrtswtbl.hxx
> @@ -219,8 +219,13 @@ inline int SwWriteTableCol::operator<( const SwWriteTableCol& rCol ) const
>       return nPos < rCol.nPos - COLFUZZY;
>   }
>
> +struct SwWriteTableColLess {
> +    bool operator()(SwWriteTableCol const * lhs, SwWriteTableCol const * rhs) {
> +        return lhs->GetPos() < rhs->GetPos();
> +    }
> +};
>
> -class SwWriteTableCols : public o3tl::sorted_vector<SwWriteTableCol*, 
> o3tl::less_ptr_to<SwWriteTableCol> > {
> +class SwWriteTableCols : public o3tl::sorted_vector<SwWriteTableCol*, SwWriteTableColLess> {
>   public:
>       ~SwWriteTableCols() { DeleteAndDestroyAll(); }
>   };