aaron.ballman 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@-1 {{integer value 8 is outside the valid range of values
[-8, 8) for this enumeration type}}
+
----------------
erichkeane wrote:
> 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.
+1, I think we should leave the version that's in this patch. It's not ideal,
but I like it better than the alternatives.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130058/new/
https://reviews.llvm.org/D130058
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
