Re: [Libreoffice] [PATCH] fdo#34908

Hi Lubos
On 25/03/11 18:52, Lubos Lunak wrote:
> On Friday 25 of March 2011, Noel Power wrote:
>
>   Ewww ...
>
>      class SAL_DLLPUBLIC_EXPORT IFieldmark
>          : virtual public IMark
> ...
>      class SAL_DLLPUBLIC_EXPORT ICheckboxFieldmark
>          : virtual public IFieldmark
> ...
> IFieldmark* pFieldmark = ...
> ...
> ICheckboxFieldmark* pCheckboxFm =
> reinterpret_cast<ICheckboxFieldmark*>(pFieldmark);
>
>
>   You really don't want to reinterpret_cast up and down virtual inheritance.
undoubtedly ( and I was uneasy about it ) but... before posting the 
patch I did check with valgrind and also printing out the result of the 
pointers from the reinterpret_cast vrs the dynamic_cast  ( on both the 
working 64 & 32 builds ) and then valgrind again on the problematic 
32bit ( with hack applied )
> Does your changing from dynamic_cast to reinterpret_cast actually really fix
> it? Since both the classes are defined in the same place, the only reasonable
> explanation I see for this is that somebody got the casting similarly wrong
> in another place and doing it wrong here too "undoes" the first wrong. I
> don't have a very good explanation why this would be different for 32/64bit
> though.
its worse as it's not just a 32bit vrs 64bit issue, its just the distro 
( patches applied ) 32 that fails, all other distro/no-distro 32/64bit 
combos work :-/
>   I don't have any 32bit build, could you try with yourself few more things?
> First, using Valgrind is always a good idea,
absolutely ( and I did that before even posting here )
>   and second, the output of
> something like this could be interesting too:
>
> printf( "%p %p %s %p %p %s %p %p %s\n", ptr, dynamic_cast<  void*>( ptr ),
> typeid( *ptr ).name(), pFieldmark, dynamic_cast<  void*>( pFieldmark ),
> typeid( *pFieldmark ).name(), pCheckboxFm, dynamic_cast<  void*>(
> pCheckboxFm ), typeid( *pCheckboxFm ).name());
>
>   (where 'ptr' is what you get from the pMarksAccess->makeNoTextFieldBookmark()
> call before casting to anything).
I'll check that later ( I  didn't know about the typeid method ) so 
thats a nice hint. Hmm I was concentrating so much on the circumstances 
of the problem I missed a safer fix, the dynamic_cast/reinterpret_cast 
is not really necessary it's more a convenience ( to be able to call the 
Set/IsChecked methods ) but these methods themselves only call 
IFieldmark related (pure) virtual methods and no ICheckboxmark related 
functions ( which might someway explain why the hack works without 
memory foobar )
Still I think there is something strange happening here, I would suspect 
that you are halfway right and there is maybe somewhere a fundamental 
error that we get away with somehow in all cases except the distro 32 
bit. But really I admit I have no clue yet. I post the results of the 
debug later when I get a chance, maybe it might throw some light

thanks again

Noel