Thank you for all the feedback -- I've made these changes and committed in r213865.
~Aaron On Tue, Jul 22, 2014 at 10:16 PM, Richard Smith <[email protected]> wrote: > 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 <[email protected]> > wrote: >> >> Latest incarnation. >> >> ~Aaron >> >> On Tue, Jul 22, 2014 at 8:50 PM, Aaron Ballman <[email protected]> >> wrote: >> > On Tue, Jul 22, 2014 at 8:45 PM, Richard Smith <[email protected]> >> > wrote: >> >> On Tue, Jul 22, 2014 at 5:38 PM, Aaron Ballman <[email protected]> >> >> wrote: >> >>> >> >>> How do these sound to you? >> >>> >> >>> On Tue, Jul 22, 2014 at 8:13 PM, Richard Smith <[email protected]> >> >>> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
