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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2420
+  constexpr E1 x2 = static_cast<E1>(8); // expected-error {{must be initialized by a constant expression}}
+  // expected-note at -1 {{integer value 8 is outside the valid range of values [-8, 8) for this enumeration type}}
+
----------------
aaron.ballman wrote:
> tahonermann wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > erichkeane wrote:
> > > > > aaron.ballman wrote:
> > > > > > erichkeane wrote:
> > > > > > > Are we ok with how subtle the `[N, M)` syntax is here?
> > > > > > FWIW, I pulled this from diagnostics like: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L9904 and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L11541
> > > > > Those aren't particularly high quality diagnostics, the first is for builtin ranges (and builtins have notoriously bad diagnostics), the 2nd is for the matrix type, which is only slightly better.
> > > > > 
> > > > > That said, if you are ok with it, I'm ok, just somewhat afraid it'll be a touch confusing.
> > > > Yeah, it's not the best diagnostic, to be sure. The trouble is that spelling it out makes it worse IMO: `integer value %0 is outside the valid range of values %1 (inclusive) and %2 (exclusive) for this enumeration type`
> > > Ok then, I can't think of anything better really (PERHAPS something that says, `integer value %0 is outside of the valid range of values (%1 - %2 inclusive) for this enumeration type`, so I'm ok living with it until someone proposes better in a followup patch.
> > > 
> > > 
> > I've never cared for the `[` vs `(` notation to indicate inclusivity vs exclusivity. All I see are unbalanced tokens and I can never remember which brace means what; I have to look it up every time and it isn't an easy search, especially for people that aren't already somewhat familiar with the notation; you have to know to search for something like "range inclusive exclusive notation". I urge use of the more elaborate diagnostic.
> I'm fine with being more verbose in the diagnostic so long as it doesn't go overboard. I don't really like the wording Erich suggested because it can be misinterpreted as both values being inclusive. I can hold my nose at what we have above. We're inconsistent in how we report this kind of information and it seems like someday we should improve this whole class of diagnostics (ones with ranges) to have a consistent display to the user. (CC @cjdb for awareness for his project, nothing actionable though.)
My intent WAS for both values to be inclusive!  That is, we'd say `integer value -8 is outside the valid range of values(0 - 7 inclusive) for this enumeration type`), but the additional logic there is likely a PITA for minor improvement.

I'm ALSO ok with holding my nose here, but would welcome a patch to improve this diagnostic (and, as Aaron said, ALL range diagnostics!). I, however, am not clever enough to come up with it.


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

https://reviews.llvm.org/D130058