Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747193002:
Fix instances of clang -Wbitfield-enum-conversion in Blink (Closed)
|
Created:
3 years, 9 months ago by Reid Kleckner Modified:
3 years, 9 months ago CC:
chromium-reviews, ojan+watch_chromium.org, shans, eae+blinkwatch, fs, zoltan1, kouhei+svg_chromium.org, rwlbuis, krit, szager+layoutwatch_chromium.org, jchaffraix+rendering, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, Eric Willigers, rjwright, atotic+reviews_chromium.org, blink-reviews-layout_chromium.org, darktears, cbiesinger, glebl+reviews_chromium.org, dgrogan+ng_chromium.org, blink-reviews-animation_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, fmalita+watch_chromium.org, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix instances of clang -Wbitfield-enum-conversion in Blink
Widen parent_writing_mode_ to fit all enumerator values. This looks like
a real bug.
Add static_cast<unsigned> when assigning m_cssPropertyId. This
suppresses the warning. It looks like enumerators larger than
lastCXXProperty should never appear here.
R=pdr@chromium.org,thakis@chromium.org
BUG=701384
Review-Url: https://codereview.chromium.org/2747193002
Cr-Commit-Position: refs/heads/master@{#456848}
Committed: https://chromium.googlesource.com/chromium/src/+/782364b2af9f1c3c7f0a39d5dd9fa1df34cdfb09
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment documenting why this cast is necessary #
Total comments: 3
Patch Set 3 : better wording #
Messages
Total messages: 21 (9 generated)
The CQ bit was checked by rnk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with the comment https://codereview.chromium.org/2747193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2747193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:75: unsigned parent_writing_mode_ : 3; ikilpatrick: Looks like this was added in https://codereview.chromium.org/2446243003/diff/60001/third_party/WebKit/Sour... but the writing mode has always needed 3 bits since it was added in https://codereview.chromium.org/2267383003/diff/160001/third_party/WebKit/Sou... I guess we're lacking test coverage in this area? https://codereview.chromium.org/2747193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp (right): https://codereview.chromium.org/2747193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp:43: m_cssPropertyId(static_cast<unsigned>(cssPropertyId)), add comment why a) cast is here b) why this is safe
lgtm with the comment
On 2017/03/14 at 17:16:55, thakis wrote: > lgtm with the comment lgtm 2, these comments sgtm 2
ikilpatrick@chromium.org changed reviewers: + ikilpatrick@chromium.org
lgtm, yup we are missing tests for sideways writing modes at the moment.
lgtm, yup we are missing tests for sideways writing modes at the moment.
The CQ bit was checked by rnk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, pdr@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2747193002/#ps20001 (title: "Add comment documenting why this cast is necessary")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(sorry) https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp (right): https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp:44: // truncation. Maybe also say why this is safe?
https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp (right): https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp:44: // truncation. On 2017/03/14 18:50:00, Nico wrote: > Maybe also say why this is safe? The DCHECK_EQ is already checking that this is safe. I'm also not really a fan of these sorts of comments. I'd rather let someone who wants to know why there's a cast try to remove it and find out what the compiler says. If someone makes this not a bitfield, for example, this comment will become stale, and the cast superfluous.
https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp (right): https://codereview.chromium.org/2747193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/properties/SVGAnimatedProperty.cpp:44: // truncation. On 2017/03/14 19:37:25, Reid Kleckner wrote: > On 2017/03/14 18:50:00, Nico wrote: > > Maybe also say why this is safe? > > The DCHECK_EQ is already checking that this is safe. > > I'm also not really a fan of these sorts of comments. I'd rather let someone who > wants to know why there's a cast try to remove it and find out what the compiler > says. If someone makes this not a bitfield, for example, this comment will > become stale, and the cast superfluous. Which dcheck? The `DCHECK_EQ(this->cssPropertyId(), cssPropertyId);`? Imho `// Safe because values that don't fit in bitfield are aliases which aren't passed to this function, see static_assert in header` or something like that makes it obvious that the author has thought about this. When i blame code, I often wonder if something's intentional the way it is or it just happens to be that way, and "just happens to be that way" is true fairly often. So I find comments like this helpful.
The CQ bit was checked by rnk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, pdr@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2747193002/#ps40001 (title: "better wording")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489521514316280,
"parent_rev": "ccb3715e9eede6f2d6f3a76654339bc8c4b4830f", "commit_rev":
"782364b2af9f1c3c7f0a39d5dd9fa1df34cdfb09"}
Message was sent while issue was closed.
Description was changed from ========== Fix instances of clang -Wbitfield-enum-conversion in Blink Widen parent_writing_mode_ to fit all enumerator values. This looks like a real bug. Add static_cast<unsigned> when assigning m_cssPropertyId. This suppresses the warning. It looks like enumerators larger than lastCXXProperty should never appear here. R=pdr@chromium.org,thakis@chromium.org BUG=701384 ========== to ========== Fix instances of clang -Wbitfield-enum-conversion in Blink Widen parent_writing_mode_ to fit all enumerator values. This looks like a real bug. Add static_cast<unsigned> when assigning m_cssPropertyId. This suppresses the warning. It looks like enumerators larger than lastCXXProperty should never appear here. R=pdr@chromium.org,thakis@chromium.org BUG=701384 Review-Url: https://codereview.chromium.org/2747193002 Cr-Commit-Position: refs/heads/master@{#456848} Committed: https://chromium.googlesource.com/chromium/src/+/782364b2af9f1c3c7f0a39d5dd9f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/782364b2af9f1c3c7f0a39d5dd9f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
