aaron.ballman added inline comments.
================ Comment at: clang/test/AST/Interp/enums.cpp:25 + SIX = FIVE + 2, + +}; ---------------- tbaeder wrote: > aaron.ballman wrote: > > tbaeder wrote: > > > aaron.ballman wrote: > > > > tbaeder wrote: > > > > > aaron.ballman wrote: > > > > > > tbaeder wrote: > > > > > > > shafik wrote: > > > > > > > > Maybe some edge case values for enumerators like `__INT_MAX__ > > > > > > > > *2U +1U` (UINT_MAX) > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > ``` > > > > > > > > enum E { // warning: enumeration values exceed range of largest > > > > > > > > integer [-Wenum-too-large] > > > > > > > > E1 = -__LONG_MAX__ -1L, > > > > > > > > E2 = __LONG_MAX__ *2UL+1UL > > > > > > > > }; > > > > > > > > ``` > > > > > > > Hm, looks like that test broke one of the windows builders: > > > > > > > https://lab.llvm.org/buildbot/#/builders/123/builds/13424 - are > > > > > > > enums larger by default on Windows? What do you suggest to fix > > > > > > > the test? > > > > > > If we're trying to be compatible with MSVC, we use their rules for > > > > > > picking the underlying type of an enumeration is which not fixed. > > > > > > One way to handle this is to add more RUN lines with explicit > > > > > > triples, but I think we lose too much interesting coverage that > > > > > > way. I'd probably use a `#ifndef _MSC_VER` block to control the > > > > > > expected diagnostics with a comment as to why the diagnostic is not > > > > > > expected on Windows. > > > > > The second builder that broke was a hexagon builder: > > > > > https://lab.llvm.org/buildbot/#/builders/38/builds/6231 - this would > > > > > still fail with the `_MSC_VER` change, right? > > > > Nope, drat. > > > > > > > > I would recommend breaking that specific test out into a separate file > > > > where we can add various RUN lines with triples, and the rest of the > > > > test can remain targetless. > > > Shame, but alright. So, what triples should I test? :) > > The ones from the failing bots, and I'd say maybe `x86_64-pc-linux` and > > `i686-pc-linux` would be reasonable. > Heh, the warning doesn't happen with the i686 triple either (with the old > interpreter). Is that expected? Yeah, that sounds about like what I'd expect. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134020/new/ https://reviews.llvm.org/D134020 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits