Bug 150537 - Use standard C/C++ integer types where relevant instead of sal_IntNN and sal_uIntNN
Summary: Use standard C/C++ integer types where relevant instead of sal_IntNN and sal_...
Status: RESOLVED NOTABUG
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium trivial
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-21 19:01 UTC by Eyal Rozenberg
Modified: 2022-08-23 06:35 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eyal Rozenberg 2022-08-21 19:01:13 UTC
The C and C++ language standard have had specific-width types since C99 and C++11 respectively :

https://en.cppreference.com/w/c/types/integer
https://en.cppreference.com/w/cpp/types/integer

and Boost has had specific-width integer types since before 2011:

boost::int_t<N>::exact
boost::uint_t<N>::exact

yet, for some reason, LO has custom code for defining specific-width type, instead of just using the standard C types in C code, and the same, but after an appropriate `using` declaration, in C++. So, some tinkering and deletions in include/sal/types.h , then using int8_t, uint8_t, int16_t, uint16_t, and so on (include size_t, the intptr_t types and 

It looks like include/sal/types.h is a relic for pre-2011 C++, and even pre-C99 C, relying on things such as `__int64` and the like.


Benefits of doing this:

* Will make the LO code a little more idiomatic C++ rather than reinventing the wheel in this respect. (Obviously there are many things which could be done to avoid idiosyncrasies, that's just one of them).
* Will shorten the code slightly: `sal_Int32` is 9 chars, `int32_t` is 7 chars. 

Detriments of doing this:

* Massive change, affects over 3,000 files
* To affect this change, the repository will probably need locking, as almost any other change to C/C++ code will likely break this one

Bikeshedding:

If type name length is a factor, or if one wants to also do this for `sal_Bool` which is trickier (no standard fixed-width boolean type in C++) - then the type names, defined using `typedef X Y;` or `using Y = X;` statements, would be `bool8`, `int8`, `uint8`, `int16`, `uint16` etc. - which would be more in line with the suffixes of the current types. In this case, name lengthes are shortened even more :-)
Comment 1 Stephan Bergmann 2022-08-22 11:34:05 UTC
The published URE interface forces us to keep (a) include/sal/types.h (for API stability), (b) the exact aliasing of the sal Int types to the underlying fundamental types (for ABI stability), and (c) the use of those sal Int types throughout the URE interface (e.g., in include/rtl/ include files).

There may be places where code not interacting with that URE interface could use std int types rather than sal Int types (and which can get cleaned up over time), but I'm not sure it's useful to track that with a bugzilla issue like this one.  (At the very least, such changes would probably not be EasyHacks.)

Closing as NOTABUG.
Comment 2 Eyal Rozenberg 2022-08-22 12:04:13 UTC
(In reply to Stephan Bergmann from comment #1)
> The published URE interface forces us to keep (a) include/sal/types.h (for
> API stability)
>, (b) the exact aliasing of the sal Int types to the
> underlying fundamental types (for ABI stability),

This is circular reasoning. You're saying you can't change the definitions in the header because you're forced not to change them so that they don't change.

I'm suggesting a change. Of course, any code which depends on the names of the types, or the text of the header, would be broken. But why is this a problem? This could be adopted in a new major version of LO.

> and (c) the use of those
> sal Int types throughout the URE interface (e.g., in include/rtl/ include
> files).

But that's part of what I'm suggesting a change in.

Finally, it's impolite to close bugs in mid-discussion.
Comment 3 Eyal Rozenberg 2022-08-22 12:52:46 UTC
And another point: include/sal/types.h 's git blame suggests that it existed in the same basic form w.r.t. integer types probably from 2000, i.e. 22 years ago. I don't think that those types are such a wonderful piece of code that it can't go up for re-evaluation once every 20 years or so.
Comment 4 Stephan Bergmann 2022-08-23 06:35:20 UTC
(In reply to Eyal Rozenberg from comment #2)
> I'm suggesting a change. Of course, any code which depends on the names of
> the types, or the text of the header, would be broken. But why is this a
> problem? This could be adopted in a new major version of LO.

It's a problem because we try to give API and ABI stability guarantees for the published URE interface.  3rd-party (extension) developers rely on that interface not changing in ways that would require them to rewrite or even rebuild their code in order to keep it working with newer versions of LO.

> Finally, it's impolite to close bugs in mid-discussion.

If you wanted to start a discussion, it would probably have been better to start it on some channel like the libreoffice@lists.freedesktop.org mailing list.