This bug seems to cause problems with merged cells.  The issue was initially detected in LibreOffice and their test case is reproduceable in AOO.

The LO fix was done by Caolan McNamara.

I will have a look at this issue.

(In reply to comment #0)
> Using OpenGrok to check the definition of nRowSpan we find it defined in
> sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> 

I did not find a <nRowSpan> defined as sal_Int32 in sw/source/filter/inc/wrtswtbl.hxx
Pedro: Can you please clarify?

> However, in file main/sw/source/filter/ww8/wrtww8.cxx, for the header of
> lcl_TCFlags() around line 1920 we see it is declared as a long.
> 
> This is likely to have negative effects, especially on 64 bit platforms.

Pedro: What would be the problem when assigning an sal_Int32 to an long? Especially on 64 bit platforms?

(In reply to comment #1)
> This bug seems to cause problems with merged cells.  The issue was initially
> detected in LibreOffice and their test case is reproduceable in AOO.
> 

Pedro: Can you attach at least one test case to this issue? Thx in advance.

> The LO fix was done by Caolan McNamara.

(In reply to comment #3)
> (In reply to comment #0)
> > Using OpenGrok to check the definition of nRowSpan we find it defined in
> > sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> > 
> 
> I did not find a <nRowSpan> defined as sal_Int32 in
> sw/source/filter/inc/wrtswtbl.hxx
> Pedro: Can you please clarify?
> 

Looking in OpenGrok there are this lines:

http://opengrok.adfinis-sygroup.org/source/search?q=&defs=nRowSpan&refs=&path=&hist=&project=aoo-trunk

65 sal_uInt16 nRowSpan;	 // ueberspannte Zeilen member in class:SwWriteTableCell 
137 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
288 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,

but it appears defined as sal_uInt16 in other places as well.

> > However, in file main/sw/source/filter/ww8/wrtww8.cxx, for the header of
> > lcl_TCFlags() around line 1920 we see it is declared as a long.
> > 
> > This is likely to have negative effects, especially on 64 bit platforms.
> 
> Pedro: What would be the problem when assigning an sal_Int32 to an long?
> Especially on 64 bit platforms?

Problem is the size: there are no guarantees that the sizes match, which can cause either that data is lost or that overflows occur and we wont catch them. 

The fix is pretty easy: just have the sizes match by replacing the long in line 1920 with sal_uInt32. It's a local variable so it won't have any negative effect.

Created attachment 79838 [details]
Test case (from LO)

Here is the LO test case:
-It is a file with a merged cell saved with MS Office.
-Open it with AOO (should look fine) and save it with another name without modifying anything.
-Close and reopen: the merged cell will appear split in two.

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > Using OpenGrok to check the definition of nRowSpan we find it defined in
> > > sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> > > 
> > 
> > I did not find a <nRowSpan> defined as sal_Int32 in
> > sw/source/filter/inc/wrtswtbl.hxx
> > Pedro: Can you please clarify?
> > 
> 
> Looking in OpenGrok there are this lines:
> 
> http://opengrok.adfinis-sygroup.org/source/
> search?q=&defs=nRowSpan&refs=&path=&hist=&project=aoo-trunk
> 
> 65 sal_uInt16 nRowSpan;	 // ueberspannte Zeilen member in
> class:SwWriteTableCell 
> 137 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
> 288 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
> 
> but it appears defined as sal_uInt16 in other places as well.
> 

Ouch.. nRowSpan is defined as sal_uInt16 on some places and
sal_Int32 on other places, and also long on a few other places
but apparently only when it is a local value.

Both sal_uInt16 and sal_Int32 types should have the same size,
but there is no guarantee for long. Does it make sense to have
a negative nRowSpan ?

Yes, changing the parameter type of <nRowSpan> from long to sal_Int32 in method <lcl_TCFlags(...)> makes sense.

@Pedro: Can you please verify the fix? Thx in advance.
Additional to the intrinsic type change the parameter is now also constant and an If-statement which checks, if the value of the parameter is negative, is removed.

I think the patch is correct but we still fail the testcase :(

@Pedro:
You are right.
The test case also fails on my system (Windows 7).
It looks like that I have overseen that <nRowSpan> values can become negative. The corresponding stl vector is defined as ::std::vector<sal_uInt32>. I thought that no negative values for <nRowSpan> could occur and removed the corresponding use case.

I will have a deeper look into the code.

My deeper look at the code and a further debugging session reveals that negative values for parameter <nRowSpan> of function <lcl_TCFlags(..)> are intended.

My former patch "went too far":
I changed the parameter type from <long> to <sal_uInt32>, because the parameter was filled with the value of an stl vector whose definition is ::std::vector<sal_uInt32>. But, actually negative values are stored inside this stl vector.

An corresponding adjustment of the code will follow:
- Change parameter type to <sal_Int32>
- Bring back the negative value use case
- Adjust the stl vector definition

revised solution for former revision 1409212 is in progress

Copying SVN Robot comment as I made a typo in the commit message - wrong issue id :(

"orw" committed SVN revision 1416474 into trunk:
#121282# - correction of former solution (revision 1409212) - consider negati...

@Pedro:
I made a correction. Can you please verify it. Thx in advance.

(In reply to comment #15)
> @Pedro:
> I made a correction. Can you please verify it. Thx in advance.

Hi;

I tested the correction and it works. Evidently nRowSpan becomes
negative when rows are merged.

Thanks!