Re: [Libreoffice] [PATCH] harmonization of BOOL vs sal_Bool as a prep for BOOL to bool converstion

On 10/4/10, Michael Meeks <michael.meeks@novell.com> wrote:
> Hi Norbert,
>
>       So - first ... thanks for your great set of patches, it is nice to see
> clean-ness trickling into the code :-) I reviewed, built, tested, and
> merged these patches:
>
>       * Convert all virtual functions QueryValue() and PutValue() to
>         return bool instead of a mix of BOOL and sal_Bool - ~1Mb ;-)
>
>       * native bool support in SvXMLUnitConverter and connexe
>         bool-related-issues
>
>       * harmonized some headers with implementation regarding BOOL
>         vs sal_Bool use.
>
>       Which all looked great. They all looked good. Of course, for this task
> we should be a bit careful about UNO compiled methods which use sal_Bool
> - although, I suspect that even they would cause little trouble [ but
> breaking the ABI related assembler binary bridges would be a little
> unfortunate ].

My goal is to push sal_Bool (i.e bool as unsigned char) as much as
possible to the boundary.
But I'm not sure where that ABI boundary is. Is there a document
somewhere that explain what is the ABI (IOW what interfaces are 'ABI
public' versus interface used only internally


>       Anyhow - I didn't merge this:
>
>       * Add support for bool for the operator << and >> of SvStream
>               + didn't apply this one: do we really need it (yet?)
>                       + was concerned about accidental usage really.
The way I am proceeding is to change
typedef sal_Bool BOOL
to
typedef bool BOOL

and then compile, find problems, fix them, then change back the typedef
re-compile - to make sure that there are still no problem
and make a patch with the change so far
Without the support of << >> fo bool, this is the first thing that
break and I would have to
re-patch them every time to move forward (and not forget to undo it
before submitting a patch...
Eventually this will be needed anyway and in the mean time, while BOOL
is typdef'ed as sal_Bool this is without impact.

>               + also, sizeof (bool) is not a good idea; if we are
>                 writing to a binary stream, we want a fixed size eg.
>                 a char on every platform, and not to change the binary
>                 format (of course).

Yes good point. I didn't realized then that sizeof(bool) is not
specified by the C++ standard
I can very easily keep the serializetion use sizeof(char).

> On Sun, 2010-10-03 at 14:39 -0500, Norbert Thiebaud wrote:
> > [PATCH] harmonization of BOOL vs sal_Bool as a prep for BOOL to bool
> > converstion
>
>       Or this attached patch, which (unfortunately) defeated me - too much
> line wrapping grief, we should really teach your mailer about that ;-)

Yep, sorry about that. I will be more careful in the future.

> Better - we should get you direct commit access, can you go through the:
> http://freedesktop.org/wiki/AccountRequests process ?

I can... but one step will be difficult: the signing of the key.
Unless you know someone in the Dallas area that can do that for me ?

how about setting up a git repo on github or something, so that you
can pull from that rather than post-processsing my emails ?

>       I'd be happy for this sort of cleanup to be committed directly, under
> three conditions:
>
>       a) we don't touch any UNO-ised methods (yet[1]) [ these are
>          generated with sal_Bool anyway ].

Again. How do I identify for sure what constitute a UNO-ised method ?

>       b) the code compiles cleanly
>
>       c) you have checked a diff before/after of:
>               'make vtable_check'
>          to ensure we didn't accidentally spike any virtual methods

What does vtable_check check (*)? How to interpret the result ?
(*) yes it check the vtables... that much I figured out :-) but bear
with me. I'm a C guy. My C++ experience is very superficial.

>       Just as an example my diff -u from before or after showed some false
> positives [ a patch from Kohei, and your sw class rename to remove the
> ambiguity ] which was interesting, I append the output for your
> delectation. A correct (pure) re-factor, that doesn't include this stuff
> would of course be an empty diff.
>
>       Anyhow - it'd be great if you could re-send your latest work without
> the line wrapping (as an attachment is perhaps best) - and an assurance
> that the vtable check passes :-)
>
>       Many thanks,
>
>               Michael.
[....]
> --
>  michael.meeks@novell.com  <><, Pseudo Engineer, itinerant idiot