[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D130058#3689053 <https://reviews.llvm.org/D130058#3689053>, @thakis wrote:

> In D130058#3687868 <https://reviews.llvm.org/D130058#3687868>, @aaron.ballman wrote:
>
>> In D130058#3687680 <https://reviews.llvm.org/D130058#3687680>, @thakis wrote:
>>
>>> Hello, this breaks a bunch of existing code over here (and I imagine elsewhere).
>>
>> Do you have an idea on how much a bunch is and whether the fixes are particularly involved or not? Did it break a system header?
>
> Sorry for the slow reply, this was fairly involved to count. (Since this is an error, not a warning, I had to do local builds on all our supported platforms, and fix all instances before I could know I found everything.)

Thank you for helping to track those numbers down, I really appreciate it!

> With D130811 <https://reviews.llvm.org/D130811>, the most annoying instance is resolved. I'd say it's now few enough instances in few enough repositories that this isn't necessary for Chromium, but based on the numbers below I'd expect that this change will be fairly annoying for people with larger codebases.
>
> For Chromium, this affects:
>
> - 6 different places in v8, all due to due using enums with a spiffy BitField class (reduced example here https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c7). This BitField class takes a "size" template arg, and clang now happens to complain if that size arg is too large to hold all enum fields: If you have an enum with 8 values but try to store it in a BitField<.., 4> you now happen to get an error, because a BitField<..., 3>. (This is kind of cool!) (https://chromium-review.googlesource.com/c/v8/v8/+/3794708)

That's neat!

> - 11 different places in chromium that are related to a macro that takes an enum (summary here: https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c11) where the workaround is to give those enums a fixed underlying type
> - 4 places in unit tests that do something like `constexpr auto kImpossibleEnumValue = static_cast<MyEnumType>(-1);` (replaced "constexpr" with "const" as workaround)
> - 1 place that checked that static_assert()ed that two enumerators from two distinct enum types have the same value, this just needed rewriting the expression slightly
>
> After D130811 <https://reviews.llvm.org/D130811>, we're lucky that no code in difficult-to-change third-party repositories are affected.

Glad to hear that!

> It does affect 22 distinct places though, so it seems likely that other projects might be less lucky.
>
> (But since it's no longer a problem for us, I also won't push for a warning more than I did in this comment.)

Thank you for these details! That does seem like a fairly substantial amount of broken code for one project, but it also sounds like everything it caught was a true positive (at least as far as the language standard is concerned) and none of it required major changes to fix the issues. Based on that, I think we're still okay with the functionality as-is, but if we start to find cases where the fix is really involved (or is in a system header) and the code is otherwise reasonable, we may want to reconsider.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130058/new/

https://reviews.llvm.org/D130058