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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.

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)

- 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.

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.)


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

https://reviews.llvm.org/D130058