Unsigned to avoid negative values
Hello! I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive. Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide? What do you think? —Myles
I could have sworn reading a few years ago a white paper discussing signed vs unsigned discussed with Blink coding style showing that using unsigned had a performance impact. Of course, now I can’t find reference to it. But I clearly recall recommendations like you mentioned. Sent from my iPhone
On 24 Jan 2023, at 9:00 pm, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Hello!
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
—Myles _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Tue, Jan 24 2023 at 02:00:11 AM -0800, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
What do you think?
This has been a best practice for a long time now. It's a good rule to reduce bugs if adopted consistently, but I also fear that if we were to try to adapt existing WebKit code to follow these guidelines, we might accidentally introduce a lot of new bugs, especially regarding container types like Vector. So I don't know what to think!
Hi, What’s the benefit? I don’t think we should be changing our long-time coding practices unless there are clear benefits from doing so. From your email, it is not yet clear to me what those benefits would be. Chris.
On Jan 24, 2023, at 6:58 AM, Michael Catanzaro via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Tue, Jan 24 2023 at 02:00:11 AM -0800, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
What do you think?
This has been a best practice for a long time now. It's a good rule to reduce bugs if adopted consistently, but I also fear that if we were to try to adapt existing WebKit code to follow these guidelines, we might accidentally introduce a lot of new bugs, especially regarding container types like Vector. So I don't know what to think!
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Tue, Jan 24, 2023 at 11:00 AM Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
What do you think?
What this immediately made me think of is that Web IDL and the web platform at large use unsigned and signed integers of various types. And as those have different value spaces you'd notice if you do something near the limits and did not carefully account for that.
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
I don’t think we should change our coding style guidelines just because C++ core guideline says something. - R. Niwa
If a value represents a size or a count or something that inherently cannot be negative, I strongly prefer using unsigned types. It reduces the number of places where we need to ask ourselves “what if it’s negative?” when it can never be negative, leading to more straightforward code that doesn’t have to handle impossible cases. It also eliminates the possibility of malicious content somehow incrementing a signed 32 bit integer past its maximum value and executing code with unexpected negative values used in signed comparison operations.
On Jan 24, 2023, at 11:44 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
I don’t think we should change our coding style guidelines just because C++ core guideline says something.
- R. Niwa
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Okay, sounds like we’re all pretty much in agreement. How about I add a rule to our style guide that says “use unsigned types to represent values which cannot be negative.” Good idea?
On Jan 25, 2023, at 4:11 PM, Alex Christensen <achristensen@apple.com> wrote:
If a value represents a size or a count or something that inherently cannot be negative, I strongly prefer using unsigned types. It reduces the number of places where we need to ask ourselves “what if it’s negative?” when it can never be negative, leading to more straightforward code that doesn’t have to handle impossible cases. It also eliminates the possibility of malicious content somehow incrementing a signed 32 bit integer past its maximum value and executing code with unexpected negative values used in signed comparison operations.
On Jan 24, 2023, at 11:44 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
I don’t think we should change our coding style guidelines just because C++ core guideline says something.
- R. Niwa
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
https://github.com/WebKit/WebKit/pull/9199
On Jan 26, 2023, at 12:31 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Okay, sounds like we’re all pretty much in agreement.
How about I add a rule to our style guide that says “use unsigned types to represent values which cannot be negative.”
Good idea?
On Jan 25, 2023, at 4:11 PM, Alex Christensen <achristensen@apple.com> wrote:
If a value represents a size or a count or something that inherently cannot be negative, I strongly prefer using unsigned types. It reduces the number of places where we need to ask ourselves “what if it’s negative?” when it can never be negative, leading to more straightforward code that doesn’t have to handle impossible cases. It also eliminates the possibility of malicious content somehow incrementing a signed 32 bit integer past its maximum value and executing code with unexpected negative values used in signed comparison operations.
On Jan 24, 2023, at 11:44 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
I don’t think we should change our coding style guidelines just because C++ core guideline says something.
- R. Niwa
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Thu, Jan 26 2023 at 12:31:25 AM -0800, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Okay, sounds like we’re all pretty much in agreement.
How about I add a rule to our style guide that says “use unsigned types to represent values which cannot be negative.”
Good idea?
This is pretty strict.
Late to the party but…. Avoiding unsigned is usually recommended to avoid inadvertent underflow: unsigned big = 200; unsigned small = 100; auto result = small - big; // underflow This is particularly bad when doing math on buffer offsets and sizes, and can result in OOB bugs. I believe Apple’s media frameworks code has a “no unsigned usage” rule because of that. I’m surprised that no-one has raised it in this discussion. Simon
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Hello!
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
—Myles _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
If you are subtracting things that have not been verified to produce a positive value, then you hopefully aren’t dealing with values that can’t be negative, so this wouldn’t apply then. Forgetting to verify things is a bug in many places. I also think that a buffer offset of -1 is just about as bad as a buffer offset of 4294967295.
On Jan 26, 2023, at 6:44 PM, Simon Fraser via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Late to the party but….
Avoiding unsigned is usually recommended to avoid inadvertent underflow:
unsigned big = 200; unsigned small = 100; auto result = small - big; // underflow
This is particularly bad when doing math on buffer offsets and sizes, and can result in OOB bugs. I believe Apple’s media frameworks code has a “no unsigned usage” rule because of that. I’m surprised that no-one has raised it in this discussion.
Simon
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Hello!
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
—Myles _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
I agree with Alex. Underflow is just as bad as overflow. —Myles
On Jan 26, 2023, at 8:12 PM, Alex Christensen <achristensen@apple.com> wrote:
If you are subtracting things that have not been verified to produce a positive value, then you hopefully aren’t dealing with values that can’t be negative, so this wouldn’t apply then. Forgetting to verify things is a bug in many places. I also think that a buffer offset of -1 is just about as bad as a buffer offset of 4294967295.
On Jan 26, 2023, at 6:44 PM, Simon Fraser via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Late to the party but….
Avoiding unsigned is usually recommended to avoid inadvertent underflow:
unsigned big = 200; unsigned small = 100; auto result = small - big; // underflow
This is particularly bad when doing math on buffer offsets and sizes, and can result in OOB bugs. I believe Apple’s media frameworks code has a “no unsigned usage” rule because of that. I’m surprised that no-one has raised it in this discussion.
Simon
On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Hello!
I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive.
Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide?
What do you think?
—Myles _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
participants (8)
-
Alex Christensen
-
Anne van Kesteren
-
Chris Dumez
-
Jean-Yves Avenard
-
Michael Catanzaro
-
Myles Maxfield
-
Ryosuke Niwa
-
Simon Fraser