Force -Werror for implicit int truncation for dom/canvas

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(7 attachments, 1 obsolete attachment)

Assignee

Description

2 months ago

We do a lot of validation, and we really don't want C++ truncating values behind our backs.

Assignee

Updated

2 months ago
Summary: Force -Werror on implicit int truncation for dom/canvas → Force -Werror for implicit int truncation for dom/canvas

It looks like you're using C-style casts, i.e.
return (uint8_t)foo;
...or
return uint8_t(expression);

Given that we're now following Google's C++ coding style (per red-background note at old Mozilla Coding Style page), perhaps these should really be using static_cast<uint8_t>(...) (or uint8_t{...}) instead?

See this particular guideline:

Do not use C-style casts. Instead, use these C++-style casts when explicit type conversion is necessary.

  • Use brace initialization to convert arithmetic types (e.g. int64{x}). This is the safest approach because code will not compile if conversion can result in information loss. The syntax is also concise.
  • Use static_cast as the equivalent of a C-style cast that does value conversion, when you need to explicitly up-cast a pointer from a class to its superclass, or when you need to explicitly cast a pointer from a superclass to a subclass. In this last case, you must be sure your object is actually an instance of the subclass.

https://google.github.io/styleguide/cppguide.html#Casting

Flags: needinfo?(jgilbert)
Assignee

Comment 10

2 months ago

I suppose so, though last time I asserted that we should follow Google C++ style, I was rebuffed in favor of existing style! My understanding is that we've only adopted the Google C++ /whitespace/ style thus far.

Also, static_cast<>() might have the worst ergonomics in the world.

Flags: needinfo?(jgilbert)
Assignee

Updated

2 months ago
Attachment #9054659 - Attachment is obsolete: true

Comment 11

2 months ago
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08d7c5e08995
Force -Werror for and fix implicit int truncation in dom/canvas. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/89552599db42
Fix implicit int truncation dom/canvas's js/* includes. r=sfink
https://hg.mozilla.org/integration/autoland/rev/54a2f08bcf48
Fix implicit int truncation in dom/canvas's dom/* includes. r=qdot
https://hg.mozilla.org/integration/autoland/rev/f4debce94b00
Fix implicit int truncation in dom/canvas's gfx/* includes. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/c941cc70ee3e
Fix implicit int conversions in dom/canvas's layout/* includes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/54d64288d7de
Fix implicit int truncation in dom/canvas's xpcom/* includes. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/093c37e856fb
Fix implicit int truncation in dom/canvas's mfbt/* includes. r=froydnj
You need to log in before you can comment on or make changes to this bug.