[Libreoffice] [PATCH] Removed dependencies on tools/solar.h

[Norbert Thiebaud]

On 1/4/11, Soeren Moeller <soerenmoeller2001 at gmail.com> wrote:
> Ok, so it seems like at the moment substituting ULONG with
> sal_uIntPtr, is the safest solution, although it maybe in a next step
> would be a good idea to look at, where it can be replaced with a
> better fitting datatype. I guess that replacing ULONG with other
> things than sal_uIntPtr also would require correcting all callsites of
> the changed functions, as this supposedly would break (or at least do
> a cast, which possible could go wrong), while the typedef in
> types/solar.h at the moment means that calling a function taking
> sal_uIntPtr with a ULONG argument still works (and hence ULONG can be
> replaced one class at a time).
>
> Do I understand correctly that I without danger can replace BOOL with
> bool in non-virtual functions,
With the caveat about UNO api.

> but should use sal_Bool or change to
> bool in all childs of the class at the same time for virtual
> functions?
today,
sal_Bool = BOOL = unsigned char

so changing BOOL to sal_Bool is always harmless.
but then the idea - as I understand it - is to try to limit sal_Bool
to UNO api related things.

There is a tools developed by michael meeks to help detect virtual
function mishap:
see http://people.gnome.org/~michael/blog/2010-10-04.html

Norbert


>
> Sören
>
> 2011/1/4 Norbert Thiebaud <nthiebaud at gmail.com>:
>> On 1/4/11, Soeren Moeller <soerenmoeller2001 at gmail.com> wrote:
>>> Thank you for your replies, for the future I will use bool when
>>> substituting BOOL outside of the UNO API.
>>>
>>> With respect to ULONG, I, as Norbert correctly observed, used
>>> sal_uIntPtr because of the typedef in tools/solar.h, but what should I
>>> use instead? It seems there are the following possibilities:
>>> - unsigned long
>>> - sal_uInt32 (could be too short on 64-bit systems)
>>
>> sal_uInt32 is probably what was originally intended with most of these
>> ULONG (back then there was no 64 bits system).
>> the problem is that changing ULONG to sal_uInt32, like changing BOOL
>> with bool can pose problem for virtual function. (as in: one need to
>> change the instance of a virtual function in all the subclass at once
>> for a given argument to avoid breaking the inheritance model)
>>
>>
>>> - sal_uIntPtr (wrong)
>> sal_uIntPtr is not wrong here. it is was is _really_ behind ULONG
>> today so that is the safest substitution, I think. Although I agree
>> with Kohei that they look weird. When I see uintptr I always think
>> that someone is using a dirty trick somewhere storing this into a
>> pointer. In other words this is misleading.
>>
>>
>>> - C99 data types (but which of these, and as Norbert writes this would
>>> require a compat.h for Microsoft compilers, and I have no idea how to
>>> do this (and no Microsoft compiler to test it))
>>
>> I favor this of course, but that would require some consensus. Either
>> we commit to use standard type or we stick with sal_xxxx, but there is
>> no benefit in mixing the 2 forever.
>> (note: in include magic is really not that hard, it can be stuck in
>> solar.h to hide MS breakages)
>>
>> Note that would still not solve the dilemma: today ULONG is defined a
>> uintptr_t so whether we use sal_uIntPtr or uintptr_t does not make any
>> difference in that matter.
>>
>> Norbert
>>
>>>
>>> I would like to do similar removements of dependencies on tools/ in
>>> other files in sc/inc/ (and the related source files), so
>>> it would be nice to know. which data types I should use instead.
>>>
>>> Regards
>>> Sören
>>>
>>>
>>> 2011/1/4 Norbert Thiebaud <nthiebaud at gmail.com>:
>>>> On Mon, Jan 3, 2011 at 10:36 PM, Kohei Yoshida <kyoshida at novell.com>
>>>> wrote:
>>>>> On Mon, 2011-01-03 at 21:47 +0100, Soeren Moeller wrote:
>>>>>> Hi
>>>>>>
>>>>>> I have removed dependencies on tools/solar.h in some files in sc
>>>>>> (according to
>>>>>> http://wiki.documentfoundation.org/Easy_Hacks#write_tools.2F_pieces_out
>>>>>> ) please review and commit.
>>>>>
>>>>> Thanks, pushed!
>>>>>
>>>>> BTW, we generally prefer the standard bool over sal_Bool, so I replaced
>>>>> sal_Bool with bool in your patch.  The only place we need to use
>>>>> sal_Bool is when dealing with the UNO API.  Other than that, the
>>>>> standard boolean type is preferred.
>>>>>
>>>>> Also, it's a bit weird to use sal_uIntPtr which isn't used much in our
>>>>> code base.  So I replaced that with sal_uInt32.
>>>>
>>>> Kohei,
>>>>
>>>> I have not read the related code, but in principle uintptr_t and
>>>> int32_t are not interchangeable.
>>>>  int32_t is 32 bit long, uintptr_t is supposed to be the same size
>>>> than void* (that is 32 or 64 bits)
>>>>
>>>> in our sources,
>>>> ULONG is typedef'ed as sal_uIntPrt (in tools/solar.h) , which is wrong
>>>> (*)
>>>> but that explain why Soeren used sal_uIntPtr.
>>>>
>>>> (*) it is wrong because there are multiple model of 64 bits support.
>>>> Notoriously, Microsoft, as usual, instead of fixing their 64 bits
>>>> support bugs, have, once again, turn their bugs into a standard and
>>>> use the so-called LLP64 model, in which sizeof(long) != sizeof(void*)
>>>> Note that ULONG is defined at multiple place, most of them as unsigned
>>>> long (which conflict with the main definition of ULONG = sal_uIntPtr).
>>>> Which raise the following question: has anyone successfully built
>>>> LibreOffice for Win64 ?
>>>>
>>>>
>>>> Note:
>>>> C99 has been a standard for quite a while now. why are we not using
>>>> the standardized type for these. that is:
>>>> int8_t uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
>>>> uint64_t, intptr_t, uintptr_t,...
>>>> see http://en.wikipedia.org/wiki/Stdint.h
>>>>
>>>> Yes, I know, Microsoft still do not have a compiler compliant with the
>>>> C-standard published 10 years ago... but that can be worked around
>>>> with a compat.h header to hide Microsoft's screw-ups, without
>>>> 'uglyfying' the rest of the code.
>>>>
>>>>>
>>>>> Kohei
>>>>>
>>>>> --
>>>>> Kohei Yoshida, LibreOffice hacker, Calc
>>>>> <kyoshida at novell.com>
>>>>>
>>>>> _______________________________________________
>>>>> LibreOffice mailing list
>>>>> LibreOffice at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/libreoffice
>>>>>
>>>>
>>>
>>
>