r213657 - Provide extra information in the "integer constant is too large" diagnostic. This will be used to improve other diagnostics.

[Richard Smith]

LGTM with tiny tweaks:

+def err_ice_too_large : Error<
+  "integer constant evaluates to value %0 that cannot be represented in a "
+  "%1-bit %select{signed|unsigned}2 integer type">;

"integer constant expression evaluates to ..." would better match standard
terminology.


+def ext_integer_literal_too_large_for_signed : ExtWarn<
+  "integer literal is too large to be represented in a signed integer
type, "
+  "and will instead be interpreted as unsigned integer type">,

You could drop the "integer type" in the last line, or maybe simply say ",
treating as unsigned" or ", interpreted as unsigned". [This is going to
look weird with -pedantic-errors (where this diagnostic is an error) but I
can't think of a way of wording this which works in that case.]

On Tue, Jul 22, 2014 at 6:04 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> Latest incarnation.
>
> ~Aaron
>
> On Tue, Jul 22, 2014 at 8:50 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> > On Tue, Jul 22, 2014 at 8:45 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >> On Tue, Jul 22, 2014 at 5:38 PM, Aaron Ballman <aaron at aaronballman.com>
> >> wrote:
> >>>
> >>> How do these sound to you?
> >>>
> >>> On Tue, Jul 22, 2014 at 8:13 PM, Richard Smith <richard at metafoo.co.uk>
> >>> wrote:
> >>> > +def err_integer_literal_too_large : Error<
> >>> > +  "integer literal is a value that cannot be represented as
> >>> > %select{a|an}0
> >>> > "
> >>> > +  "%select{signed|unsigned}0 integer">;
> >>> >
> >>> > I don't think the "is a value that" adds anything here. Also, I'd
> like
> >>> > to
> >>> > see something that more directly says we don't have a signed/unsigned
> >>> > integer type large enough.
> >>>
> >>> "integer literal is too large to be represented in %select{a|an}0
> >>> %select{signed|unsigned}0 integer type"
> >>
> >>
> >> Perhaps "... to be represented in any %select{signed |}0integer type"?
> >
> > Works for me.
> >
> >>
> >>>
> >>> >  def err_integer_too_large : Error<
> >>> > -  "integer constant is larger than the largest %0-bit unsigned
> integer
> >>> > type">;
> >>> > -def ext_integer_too_large_for_signed : ExtWarn<
> >>> > -  "integer constant is larger than the largest %0-bit signed integer
> >>> > type">,
> >>> > +  "integer constant evaluates to value %0 that cannot be
> represented as
> >>> > a "
> >>> > +  "%1-bit %select{signed|unsigned}2 integer">;
> >>> >
> >>> > Please rename this to something about ICEs, and move it to
> >>> > DiagnosticSemaKinds next to the existing err_ice_ diagnostics. (This
> is
> >>> > pretty similar to ext_cce_narrowing...)
> >>> >
> >>> >
> >>> > +def ext_integer_literal_too_large_for_signed : ExtWarn<
> >>> > +  "integer literal is a value that cannot be represented as a signed
> >>> > integer, "
> >>> > +  "and will instead be interpreted as unsigned">,
> >>> >
> >>> > Again, mentioning integer types and not just integers would make this
> >>> > clearer.
> >>>
> >>> "integer literal is too large to be represented in a signed integer
> >>> type, and will instead be interpreted as unsigned integer type"
> >>
> >>
> >> Can we specify the type here? I think it's always 'unsigned long long'
> when
> >> we reach this diagnostic.
> >
> > It's not always unsigned long long -- this gets used in EvaluateValue
> > in PPExpressions.cpp, and we only set the resulting value to unsigned,
> > but not change its type.
>

Setting it to unsigned is effectively changing its type from intmax_t to
uintmax_t (we don't model types in the preprocessor, so there's nothing
else to update here). Ideally I'd still like for us to talk about a type in
this diagnostic but your patch is fine to commit without that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140722/2ce7da7d/attachment.html>