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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
