[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}}
+
----------------
shafik wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > erichkeane wrote:
> > > > royjacobson wrote:
> > > > > 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.)
> > > > > Maybe `[%1 <= x < %2]`? Feels a bit clumsy, but it disambiguates
> > > > 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.
> > > While I like `[%1, %2)` (because I nerd out over maths), I think `%1 <= x < %2` will be more accessible to folks who haven't taken university calculus or discrete maths.
> > > 
> > > For @tahonermann specifically: a potential mnemonic is that closed intervals use a straight line, which intersects an axis, whereas open intervals are curved, which represents them being asymptotic.
> > As far as wording goes, I think `%1 <= x < %2`  is reasonable (I really don't like that `x` in there though -- the chances of that being the user's variable are very slim right up until `x` happens to be something contextually baffling like the name of a template type parameter. However, I don't see any diagnostics using that kind of wording either, so this would be adding another variant of expressing a range of values (not a huge issue, but a bit unfortunate for users).
> > 
> > Here's an idea that may be worse than anything anyone else has come up with. Split the diagnostic into two parts:
> > 
> > `integer value %0 is %select{less than the smallest|greater than the largest}1 possible value %2 for this enumeration type`
> > 
> I agree having the `x` in the diagnostic could be confusing based on the context.
> 
> I could make sure both value of the range are inclusive and go with wording like:
> 
> `integer value %0 is outside the valid range of values (%1 through %2) for this enumeration type`
IMO, we've rat-holed this more than enough, and I see nothing that is 'better enough' than the version in the patch we have to warrant diverging from other diagnostic precedence.

There _IS_ an opportunity for someone to come along with a future patch to fix all of our range-diagnostics in a BETTER way, but I don't think this is the patch to do so, nor do we have the way of doing so identified here.


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

https://reviews.llvm.org/D130058