Coding style 🙄 : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

Recently I coded something with a not-very-important slow-changing rarely-used positive number: `unsigned mGeneration;`
My reviewer commented: "Please use a type with an explicit size, such as uint32_t. (General mozilla style; you don't see a bare "unsigned" around much)"

I had never heard of this (in 4+ years), so I did a bit of research:

- I found plenty of `unsigned`s around, more than `uint32_t`s.

- I can't see anything about that in our coding style guides [1, 2].

- Our latest coding style [1] points at Google's, which has a section about Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" numbers, int64_t for "big" numbers, intXX_t if you need a precise size; never use any unsigned type unless you work with bitfields or need 2^N overflow (in particular, don't use unsigned for always-positive numbers, use signed and assertions instead).

So, questions:
1. Do we have a written style I missed somewhere?
2. Do we have an unwritten style? (In which case I think we should write it down.)
3. What do we think of the Google style, especially the aversion to unsigned?

Cheers,
Gerald


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
[3] https://google.github.io/styleguide/cppguide.html#Integer_Types

[David Teller]

The Google style sounds pretty good to me.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Boris Zbarsky]

On 7/4/19 10:11 PM, Gerald Squelart wrote:
> - I found plenty of `unsigned`s around, more than `uint32_t`s.

How many are in code that predates the ability to use uint32_t, though?

> - Our latest coding style [1] points at Google's, which has a section about Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" numbers

If you can 100% guarantee that they will not be too big, right?

(In particular, for generation counters I would be somewhat worried
about making such a guarantee.)

> never use any unsigned type unless you work with bitfields or need 2^N overflow (in particular, don't use unsigned for always-positive numbers, use signed and assertions instead).

Do you happen to know why? Is this due to worries about underflow or
odd behavior on subtraction or something?

-Boris

[Henri Sivonen]

On Thu, Jul 4, 2019 at 9:55 AM Boris Zbarsky <bzba...@mit.edu> wrote:
> > never use any unsigned type unless you work with bitfields or need 2^N overflow (in particular, don't use unsigned for always-positive numbers, use signed and assertions instead).
>
> Do you happen to know why? Is this due to worries about underflow or
> odd behavior on subtraction or something?

I don't _know_, but most like they want to benefit from optimizations
based on overflow being UB.

--
Henri Sivonen
hsiv...@mozilla.com

[Gerald Squelart]

On Thursday, July 4, 2019 at 4:53:34 PM UTC+10, Boris Zbarsky wrote:
> On 7/4/19 10:11 PM, Gerald Squelart wrote:
> > - I found plenty of `unsigned`s around, more than `uint32_t`s.
>
> How many are in code that predates the ability to use uint32_t, though?

I didn't do deeper archaeology, so it's very possible most unsigneds are in old or 3rd party code. But I didn't want to spend more time on this, in case old-timers already had knowledge about it. ;-)

> > - Our latest coding style [1] points at Google's, which has a section about Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" numbers
>
> If you can 100% guarantee that they will not be too big, right?
>
> (In particular, for generation counters I would be somewhat worried
> about making such a guarantee.)

They did add "use int64_t for "big" numbers".

In my own selfish case, it will be once per profiler start/stop. I'm fairly confident a user won't start and stop the profiler a few billion times in a session. :-D

But that was not the point of this discussion, sorry for the distraction.

> > never use any unsigned type unless you work with bitfields or need 2^N overflow (in particular, don't use unsigned for always-positive numbers, use signed and assertions instead).
>
> Do you happen to know why? Is this due to worries about underflow or
> odd behavior on subtraction or something?

Details in OP[3]. Some juicy bits:
"Because of historical accident, the C++ standard also uses unsigned integers to represent the size of containers - many members of the standards body believe this to be a mistake, but it is effectively impossible to fix at this point."
"The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler."
"Mixing signedness of integer types is responsible for an equally large class of problems."

> -Boris

g.

[Botond Ballo]

On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen <hsiv...@mozilla.com> wrote:
> > Do you happen to know why? Is this due to worries about underflow or
> > odd behavior on subtraction or something?
>

> I don't _know_, but most like they want to benefit from optimizations
> based on overflow being UB.

My understanding is yes, that's one of the motivations.

Another, as hinted at in Gerald's quote, is that tools like UBSan can
diagnose and catch signed overflow because it's undefined behaviour.
They can't really do that for unsigned overflow because, since that's
defined to wrap, for all the tool knows the code author intended for
the overflow and wrapping to occur.

Cheers,
Botond

[Jeff Gilbert]

I really, really like unsigned types, to the point of validating and
casting into unsigned versions for almost all webgl code. It's a huge
help to have a compile-time constraint that values can't be negative.
(Also webgl has implicit integer truncation warnings-as-errors, so we
don't really worry about mixed-signedness)

If we insist on avoiding standard uint types, I'll be writing uint31_t wrappers.

If we're going to recommend against uint types, I would like to see
specific compelling examples of problems with them, not just prose
about "many people say" or "maybe missed optimizations".

On Thu, Jul 4, 2019 at 8:11 AM Botond Ballo <bba...@mozilla.com> wrote:
>
> On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen <hsiv...@mozilla.com> wrote:

> > > Do you happen to know why? Is this due to worries about underflow or
> > > odd behavior on subtraction or something?
> >

> > I don't _know_, but most like they want to benefit from optimizations
> > based on overflow being UB.
>
> My understanding is yes, that's one of the motivations.
>
> Another, as hinted at in Gerald's quote, is that tools like UBSan can
> diagnose and catch signed overflow because it's undefined behaviour.
> They can't really do that for unsigned overflow because, since that's
> defined to wrap, for all the tool knows the code author intended for
> the overflow and wrapping to occur.
>
> Cheers,
> Botond

[Botond Ballo]

On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert <jgil...@mozilla.com> wrote:
> It's a huge
> help to have a compile-time constraint that values can't be negative.

The question is, how useful is that guarantee. Suppose you have some
code that decrements an integer too far, past zero. Instead of having
a -1 you'll have a 4294967295. Is that an improvement? Will it give
the code saner behaviour than the -1?

Cheers,
Botond

[Nathan Froyd]

The LLVM development list has been having a similar discussion,
started by a proposal to essentially follow the Google style guide:

http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html

The initial email has links you can follow for more information. I
recommend starting here:

https://www.youtube.com/watch?v=yG1OZ69H_-o&feature=youtu.be&t=2249

Both for the "why is unsigned arithmetic problematic at scale"
(spoiler: you can't check for bad things happening automatically) and
an example of "what sort of optimizations are you giving up".

Chandler (the speaker above) has a response that is worth reading
(noting that objections like yours and otherwise are addressed by the
links in the original email):

http://lists.llvm.org/pipermail/llvm-dev/2019-June/133023.html

-Nathan

On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert <jgil...@mozilla.com> wrote:
>

> I really, really like unsigned types, to the point of validating and

> casting into unsigned versions for almost all webgl code. It's a huge

> help to have a compile-time constraint that values can't be negative.

> (Also webgl has implicit integer truncation warnings-as-errors, so we
> don't really worry about mixed-signedness)
>
> If we insist on avoiding standard uint types, I'll be writing uint31_t wrappers.
>
> If we're going to recommend against uint types, I would like to see
> specific compelling examples of problems with them, not just prose
> about "many people say" or "maybe missed optimizations".
>
> On Thu, Jul 4, 2019 at 8:11 AM Botond Ballo <bba...@mozilla.com> wrote:
> >
> > On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen <hsiv...@mozilla.com> wrote:

> > > > Do you happen to know why? Is this due to worries about underflow or
> > > > odd behavior on subtraction or something?
> > >

[Jeff Gilbert]

That's what CheckedInt is for, and that's what we use.

The problems webgl deals with aren't arithmatic. Arithmatic is easy.
(CheckedInt!) Reasoning about constraints is hard.

We have some entrypoints where negative values are valid, and many
where they are not. It's really nice to have a natural way to document
which we expect /at compile time/. Saying "no unsigned types" really
throws out the baby with the bathwater for me.

On Thu, Jul 4, 2019 at 11:46 AM Botond Ballo <bba...@mozilla.com> wrote:
>
> On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert <jgil...@mozilla.com> wrote:
> > It's a huge
> > help to have a compile-time constraint that values can't be negative.
>

[Mats Palmgren]

On 7/4/19 1:11 PM, Henri Sivonen wrote:
> I don't _know_, but most like they want to benefit from optimizations
> based on overflow being UB.

It's worth noting that such optimizations can be exploitable if an
overflow do occur. See bug 1292443 for an example.

Compiling with -fwrapv would fix that, but then you'd lose those
optimizations of course.

/Mats

[Mats Palmgren]

On 7/4/19 1:11 PM, Henri Sivonen wrote:

> I don't _know_, but most like they want to benefit from optimizations
> based on overflow being UB.

[Gerald Squelart]

(Glad I started this discussion; thank you Nathan for the enlightening links, I need to review all my code now!)

Jeff, maybe what we need is a new value type that advertizes that it's unsigned, but doesn't have the unwanted 2^N wrapping (and its effects on bug-finding tools and compiler optimizations).
`class Unsigned { int mValue; /* magic API here */ }` -- feels like unsigned, but underneath it's all `int` arithmetics, with optional >=0 assertions.
Would that help?

Gerald

[Jeff Gilbert]

Yes I intend to write precisely that, if we ban unsigned types.
However I'm not really convinced that throwing out unsigned types is
the right move.

For instance, one of the optimizations mentioned in the linked video
seems to not mention that using (unsigned!) size_t instead of uint32_t
(like you're sort of supposed to) should also improve code generation
in the same way as using int32_t.

It is, however, super poignant to me that uint32_t-indexing-on-x64 is
pessimal, as that's precisely what our ns* containers (nsTArray) use
for size, /unlike/ their std::vector counterparts, which will be using
the more-optimal size_t.

I'm not saying there's nothing wrong with unsigned types, but rather
than I feel their faults are being overblown, and the arguments
against them feel cherry-picked, which in a language like C++, is easy
to do.

[Nathan Froyd]

On Fri, Jul 5, 2019 at 2:48 AM Jeff Gilbert <jgil...@mozilla.com> wrote:
> It is, however, super poignant to me that uint32_t-indexing-on-x64 is
> pessimal, as that's precisely what our ns* containers (nsTArray) use
> for size, /unlike/ their std::vector counterparts, which will be using
> the more-optimal size_t.

nsTArray uses size_t for indexing since bug 1004098.

-Nathan

[Henri Sivonen]

We should probably endorse the use of size_t more explicitly in our
guidelines. Apart from the issue of object size motivating especially
strings using uint32_t for _fields_, it seems to me that a significant
part of our uint32_t (originally PRUint32, of course) habit comes from
the days when Tru64 Unix was the main 64-bit Gecko platform,
therefore, we lacked proper 64-bit testing coverage.

--
Henri Sivonen
hsiv...@mozilla.com

[Ehsan Akhgari]

On Fri, Jul 5, 2019 at 2:48 AM Jeff Gilbert <jgil...@mozilla.com> wrote:

> Yes I intend to write precisely that, if we ban unsigned types.
> However I'm not really convinced that throwing out unsigned types is
> the right move.
>

Note that such a class, if it performs assertions, is actually completely
different than C++'s built-in unsigned types. For example:

unsigned i = -1; // compiles and runs just fine with -Wall
Unsigned j = -1; // would (fatally?) assert at runtime

The assumption that unsigned types can't take negative values is generally
false, unless if the values they're being assigned to are all asserted to
be non-negative...

> For instance, one of the optimizations mentioned in the linked video
> seems to not mention that using (unsigned!) size_t instead of uint32_t
> (like you're sort of supposed to) should also improve code generation
> in the same way as using int32_t.
>

It does <https://youtu.be/yG1OZ69H_-o?t=2605>, it talks about the problem
being caused by the mismatch between the size of the pointers (64 bits) vs
the indices (32 bits) and it mentions that using 32-bit indices is common
in real-world code where for example the index is stored in a data
structure and the programmer chooses 32-bits to represent the index to save
space in the data structure since they decide that the index can never be
large enough to warrant 64-bits.

> It is, however, super poignant to me that uint32_t-indexing-on-x64 is
> pessimal, as that's precisely what our ns* containers (nsTArray) use
> for size, /unlike/ their std::vector counterparts, which will be using
> the more-optimal size_t.
>

> I'm not saying there's nothing wrong with unsigned types, but rather
> than I feel their faults are being overblown, and the arguments
> against them feel cherry-picked, which in a language like C++, is easy
> to do.
>

As far as I understand these faults are the inherent semantics of unsigned
types irrespective of whether we like them or not (their modular arithmetic
semantics as well as the semantics of arithmetic with mixtures of signed
and unsigned types), and the problem is that these types do something that
programmers may not expect (e.g. "unsigned can't accept negative values",
"unsigned values will remain (correct) positive values during arithmetic
operations" etc.). The existing solutions that we have to mitigate some of
these problems (CheckedInt for example) are usually helpful when the code
author/reviewer are consciously thinking about these issues and otherwise
we end up using them in response to bug reports.

The arguments being overblown or not, IMO, is completely subjective and I
doubt it is something that we will ever agree on in a discussion forum with
many participants. :-) If you disagree with the semantic differences
about them I'd be interested to know why.

>
> On Thu, Jul 4, 2019 at 4:45 PM Gerald Squelart <gsqu...@mozilla.com>
> wrote:
> >

> > _______________________________________________
> > dev-platform mailing list
> > dev-pl...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

--
Ehsan

[Ehsan Akhgari]

On Thu, Jul 4, 2019 at 8:05 AM Gerald Squelart <gsqu...@mozilla.com>
wrote:

> > > - Our latest coding style [1] points at Google's, which has a section
> about Integer Types [3], and the basic gist is: Use plain `int` for
> "not-too-big" numbers
> >
> > If you can 100% guarantee that they will not be too big, right?
> >
> > (In particular, for generation counters I would be somewhat worried
> > about making such a guarantee.)
>
> They did add "use int64_t for "big" numbers".
>
> In my own selfish case, it will be once per profiler start/stop. I'm
> fairly confident a user won't start and stop the profiler a few billion
> times in a session. :-D
>

FWIW once in a while I have come across bugs caused by truncation of
integers where someone picked a specific size that was too small also, e.g.
storing an offset into a text node in a 16-bit integer. I think that's
maybe something that's hiding between the lines there, being careful with
that direction also if you pick a type with a specific size to make sure
your type is large enough.

--
Ehsan

[Gijs Kruitbosch]

Yep. Recent example: https://bugzilla.mozilla.org/show_bug.cgi?id=1556019 .

~ Gijs

[Chris Peterson]

On 7/5/2019 10:39 AM, Gijs Kruitbosch wrote:
>> FWIW once in a while I have come across bugs caused by truncation of
>> integers where someone picked a specific size that was too small also,
>> e.g.
>> storing an offset into a text node in a 16-bit integer.  I think that's
>> maybe something that's hiding between the lines there, being careful with
>> that direction also if you pick a type with a specific size to make sure
>> your type is large enough.
>
> Yep. Recent example: https://bugzilla.mozilla.org/show_bug.cgi?id=1556019 .

If integer truncation bugs are something we're really concerned about,
clang 8 added a new -Wimplicit-int-conversion (and
-Wimplicit-float-conversion) warning. Unfortunately, there are a couple
thousand instances of these warnings in mozilla-central. I don't know if
fixing them is practical, but they could be selectively enabled (or
disabled) for individual directories.

https://clang.llvm.org/docs/DiagnosticsReference.html#wimplicit-int-conversion

warning: higher order bits are zeroes after implicit conversion
warning: implicit conversion loses integer precision: A to B

warning: implicit conversion loses floating-point precision: A to B
warning: implicit conversion when assigning computation result loses
floating-point precision: A to B

[Kip Gilbert]

Hello!

Just wish to chime in with my 2c... Would the proposed shift towards
signed types only be for larger values (eg, >= 32 bits)?

Audio and rendering code would still require using unsigned types,
especially when packed into buffers. (eg, 8-bit unsigned color components,
32-bit packed RGBA values, 16-bit audio samples).

If just talking about improving bounds checking for array counts, and loop
iterators, this would be a different story...

If we were to replace uint64_t and size_t with int64_t's, would we take
such bounds checking further and check for other wrap-around issues
inherent to such signed / 2's compliment values? Would such checks be more
effective than just asserting that a uint64_t isn't greater than the
expected maximum value to catch wraparound?

- Kip

On Fri, Jul 5, 2019 at 11:15 AM Chris Peterson <cpet...@mozilla.com>
wrote:

[Jeff Gilbert]

dom/canvas has enabled -Werror=implicit-int-conversion since 68! :)
https://bugzilla.mozilla.org/show_bug.cgi?id=1540357

On Fri, Jul 5, 2019 at 11:15 AM Chris Peterson <cpet...@mozilla.com> wrote:
>

[Gerald Squelart]

Thank you all for some very interesting discussions so far.

Even if we don't take blanket steps to avoid unsigned types in non-bitfield/modulo cases (as suggested by our newly-adopted Google style), at least hopefully we're now aware of their subtleties, and we can be more careful and deliberate in our choice of integer types in our respective domains.

Coming back to my original questions, I think the first part has not been categorically answered yet:

Do we have style rules (or folklore) against naked `int`s/`unsigned`s, in favor of explicitly-sized `(u)intXX_t` everywhere?

Thanks,
Gerald

[Ehsan Akhgari]

On Mon, Jul 8, 2019 at 11:00 PM Gerald Squelart <gsqu...@mozilla.com>
wrote:

> Thank you all for some very interesting discussions so far.
>
> Even if we don't take blanket steps to avoid unsigned types in
> non-bitfield/modulo cases (as suggested by our newly-adopted Google style),
> at least hopefully we're now aware of their subtleties, and we can be more
> careful and deliberate in our choice of integer types in our respective
> domains.
>
> Coming back to my original questions, I think the first part has not been
> categorically answered yet:
>
> Do we have style rules (or folklore) against naked `int`s/`unsigned`s, in
> favor of explicitly-sized `(u)intXX_t` everywhere?
>

For new code, the style guide for this question can be found here:
https://google.github.io/styleguide/cppguide.html#Integer_Types. For
existing code, consistency with surrounding code should take precedence for
now. I hope this answers your question.

Cheers,
Ehsan

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

--
Ehsan

[Mike Hommey]

On Tue, Jul 09, 2019 at 10:39:37AM -0400, Ehsan Akhgari wrote:
> On Mon, Jul 8, 2019 at 11:00 PM Gerald Squelart <gsqu...@mozilla.com>
> wrote:
>

> > Thank you all for some very interesting discussions so far.
> >
> > Even if we don't take blanket steps to avoid unsigned types in
> > non-bitfield/modulo cases (as suggested by our newly-adopted Google style),
> > at least hopefully we're now aware of their subtleties, and we can be more
> > careful and deliberate in our choice of integer types in our respective
> > domains.
> >
> > Coming back to my original questions, I think the first part has not been
> > categorically answered yet:
> >
> > Do we have style rules (or folklore) against naked `int`s/`unsigned`s, in
> > favor of explicitly-sized `(u)intXX_t` everywhere?
> >
>

> For new code, the style guide for this question can be found here:
> https://google.github.io/styleguide/cppguide.html#Integer_Types. For
> existing code, consistency with surrounding code should take precedence for
> now. I hope this answers your question.

I thought we only adopted the Google style guide for formatting. Does
everything from the guide apply now? Or only parts of it? If the latter,
which parts? I'm surprised because I don't remember having seen a mail
about this, and surely, I should have noticed something that'd be
saying that class member variables names would stop beginning with m,
and would instead finish with an underscore and be all lowercase.

Mike

[Bobby Holley]

On Tue, Jul 9, 2019 at 3:23 PM Mike Hommey <m...@glandium.org> wrote:

> On Tue, Jul 09, 2019 at 10:39:37AM -0400, Ehsan Akhgari wrote:
> > On Mon, Jul 8, 2019 at 11:00 PM Gerald Squelart <gsqu...@mozilla.com>
> > wrote:
> >

> > > Thank you all for some very interesting discussions so far.
> > >
> > > Even if we don't take blanket steps to avoid unsigned types in
> > > non-bitfield/modulo cases (as suggested by our newly-adopted Google
> style),
> > > at least hopefully we're now aware of their subtleties, and we can be
> more
> > > careful and deliberate in our choice of integer types in our respective
> > > domains.
> > >
> > > Coming back to my original questions, I think the first part has not
> been
> > > categorically answered yet:
> > >
> > > Do we have style rules (or folklore) against naked `int`s/`unsigned`s,
> in
> > > favor of explicitly-sized `(u)intXX_t` everywhere?
> > >
> >

> > For new code, the style guide for this question can be found here:
> > https://google.github.io/styleguide/cppguide.html#Integer_Types. For
> > existing code, consistency with surrounding code should take precedence
> for
> > now. I hope this answers your question.
>
> I thought we only adopted the Google style guide for formatting. Does
> everything from the guide apply now? Or only parts of it? If the latter,
> which parts? I'm surprised because I don't remember having seen a mail
> about this, and surely, I should have noticed something that'd be
> saying that class member variables names would stop beginning with m,
> and would instead finish with an underscore and be all lowercase.
>

>From the original announcement [1]:

> We will automatically enforce restrictions on formatting of whitespace
(such as indentation and braces).
> For stylistic features other than that (such as naming of functions and
#include order), Google C++ style
> will be permitted but not initially enforced, and consistency with
surrounding code should take precedence.

In other words, we should default to using Google C++ style when doing so
would not be massively more disruptive or inconsistent than the
alternative. So we're not boiling the ocean over mFoo, but preferring the
explicit integer types and citing the Google style guide is a reasonable
thing to do.

[1]
https://docs.google.com/document/d/1CTaWucldHxEri5BUB1kL4faF4prwPlBa31yHFd-l8uc/edit


>
> Mike

[Gerald Squelart]

That answers my question, thank you Ehsan and Bobby.