Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-10 Thread Gerald Squelart
On Wednesday, July 10, 2019 at 9:12:23 AM UTC+10, Bobby Holley wrote:
> On Tue, Jul 9, 2019 at 3:23 PM Mike Hommey  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 
> > > 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

That answers my question, thank you Ehsan and Bobby.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-09 Thread Bobby Holley
On Tue, Jul 9, 2019 at 3:23 PM Mike Hommey  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 
> > 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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-09 Thread 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 
> 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-09 Thread Ehsan Akhgari
On Mon, Jul 8, 2019 at 11:00 PM Gerald Squelart 
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


>
> On Thursday, July 4, 2019 at 3:11:27 PM UTC+10, Gerald Squelart wrote:
> > 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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>


-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-08 Thread 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


On Thursday, July 4, 2019 at 3:11:27 PM UTC+10, Gerald Squelart wrote:
> 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread 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  wrote:
>
> 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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread 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 
wrote:

> 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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread 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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread Gijs Kruitbosch

On 05/07/2019 17:36, Ehsan Akhgari wrote:

On Thu, Jul 4, 2019 at 8:05 AM Gerald Squelart 
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.


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

~ Gijs
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread Ehsan Akhgari
On Thu, Jul 4, 2019 at 8:05 AM Gerald Squelart 
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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread Ehsan Akhgari
On Fri, Jul 5, 2019 at 2:48 AM Jeff Gilbert  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 , 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 
> wrote:
> >
> > (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
> >
> >
> > On Friday, July 5, 2019 at 5:35:30 AM UTC+10, Jeff Gilbert wrote:
> > > 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 
> wrote:
> > > >
> > > > On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert 
> 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
> >
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>


-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org

Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread Henri Sivonen
On Fri, Jul 5, 2019 at 1:28 PM Nathan Froyd  wrote:
>
> On Fri, Jul 5, 2019 at 2:48 AM Jeff Gilbert  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.

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
hsivo...@mozilla.com
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread Nathan Froyd
On Fri, Jul 5, 2019 at 2:48 AM Jeff Gilbert  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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-05 Thread 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.

On Thu, Jul 4, 2019 at 4:45 PM Gerald Squelart  wrote:
>
> (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
>
>
> On Friday, July 5, 2019 at 5:35:30 AM UTC+10, Jeff Gilbert wrote:
> > 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  wrote:
> > >
> > > On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert  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
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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


On Friday, July 5, 2019 at 5:35:30 AM UTC+10, Jeff Gilbert wrote:
> 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  wrote:
> >
> > On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert  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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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  wrote:
>
> On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert  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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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=youtu.be=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  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  wrote:
> >
> > On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen  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
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread Botond Ballo
On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert  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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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  wrote:
>
> On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen  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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread Botond Ballo
On Thu, Jul 4, 2019 at 7:11 AM Henri Sivonen  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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread Henri Sivonen
On Thu, Jul 4, 2019 at 9:55 AM Boris Zbarsky  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
hsivo...@mozilla.com
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

2019-07-04 Thread David Teller
The Google style sounds pretty good to me.

On 04/07/2019 07:11, Gerald Squelart wrote:
> 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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
> 
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


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

2019-07-03 Thread 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform