cjdb 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:
> 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.
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
