<div dir="ltr">Thanks!<div><br></div><div>Maybe it's interesting if I list the instances where this fired in Chromium.</div><div><br></div><div>* libwebp calls _mm_set1_epi16(33050) (but then is careful to only use unsigned intrinsics with this). There's no version of this function that takes unsigned short, so the only thing to change here was to add a cast.</div><div><br></div><div>* Some function taking int was passed <span style="font-size:12.8px;white-space:pre-wrap">2651229008. I don't understand why; it was some machine_id thing. The parameter probably wanted to be unsigned.</span></div><div><span style="font-size:12.8px;white-space:pre-wrap"><br></span></div><div><span style="font-size:12.8px;white-space:pre-wrap">* In two cases, a char array contained a 255 (this no longer fires after your change)</span></div><div><span style="font-size:12.8px;white-space:pre-wrap"><br></span></div><div><span style="font-size:12.8px;white-space:pre-wrap">* some file had `</span><span style="font-size:12.8px;white-space:pre-wrap">volatile short lineno = (__LINE__ << 8) + __COUNTER__; (void)lineno;` in a macro to prevent the compiler from combining functions (in dynamic_annotations.c) and that number became larget than 32k (but not larger than 64k, so we changed that to unsigned short).</span></div><div><span style="font-size:12.8px;white-space:pre-wrap"><br></span></div><div><span style="font-size:12.8px;white-space:pre-wrap">* finally we had the line `</span><span style="font-size:12.8px;white-space:pre-wrap">const int16_t kReservedManufacturerID = 1 << 15;`</span></div><div><span style="font-size:12.8px;white-space:pre-wrap"><br></span></div><div><span style="font-size:12.8px;white-space:pre-wrap">None of these were bugs, but it seems like a good change anyhow. I can see how this could catch bugs.</span></div><div><span style="font-size:12.8px;white-space:pre-wrap"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 5, 2016 at 6:07 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">r259947 will stop this warning on char arrays.<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 1, 2016 at 1:40 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">C++11 narrowing only happens during certain conversions while this warning checks all conversions.<div><br></div><div>We might be able to avoid char array initialization, but it would be a little tricky. These warning usually have little information about where the conversion is coming from, so distinguishing array initialization versus variable initialization could be hard.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jan 30, 2016 at 6:41 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Shouldn't the new case be in -Wc++11-narrowing instead? This is warning in similar places where that warning used to warn.<div><br></div><div>In practice, char arrays seem to be often used for storing binary blobs in header files, and those are usually initialized with numbers 0...255 instead of -128...127 (why not make the array uint8_t then? Because maybe the function you want to pass the data from wants a char* array, and having to reinterpret_cast from uint8_t to char is annoying). Maybe this shouldn't fire for char arrays?</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 29, 2016 at 6:51 PM, Richard Trieu via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rtrieu<br>
Date: Fri Jan 29 17:51:16 2016<br>
New Revision: 259271<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=259271&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=259271&view=rev</a><br>
Log:<br>
Improve -Wconstant-conversion<br>
<br>
Switch the evaluation from isIntegerConstantExpr to EvaluateAsInt.<br>
EvaluateAsInt will evaluate more types of expressions than<br>
isIntegerConstantExpr.<br>
<br>
Move one case from -Wsign-conversion to -Wconstant-conversion. The case is:<br>
1) Source and target types are signed<br>
2) Source type is wider than the target type<br>
3) The source constant value is positive<br>
4) The conversion will store the value as negative in the target.<br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaChecking.cpp<br>
cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp<br>
cfe/trunk/test/Sema/constant-conversion.c<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271&r1=259270&r2=259271&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271&r1=259270&r2=259271&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 29 17:51:16 2016<br>
@@ -7578,7 +7578,7 @@ void CheckImplicitConversion(Sema &S, Ex<br>
// If the source is a constant, use a default-on diagnostic.<br>
// TODO: this should happen for bitfield stores, too.<br>
llvm::APSInt Value(32);<br>
- if (E->isIntegerConstantExpr(Value, S.Context)) {<br>
+ if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {<br>
if (S.SourceMgr.isInSystemMacro(CC))<br>
return;<br>
<br>
@@ -7603,6 +7603,42 @@ void CheckImplicitConversion(Sema &S, Ex<br>
return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_precision);<br>
}<br>
<br>
+ if (TargetRange.Width == SourceRange.Width && !TargetRange.NonNegative &&<br>
+ SourceRange.NonNegative && Source->isSignedIntegerType()) {<br>
+ // Warn when doing a signed to signed conversion, warn if the positive<br>
+ // source value is exactly the width of the target type, which will<br>
+ // cause a negative value to be stored.<br>
+<br>
+ llvm::APSInt Value;<br>
+ if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {<br>
+ if (!S.SourceMgr.isInSystemMacro(CC)) {<br>
+<br>
+ IntegerLiteral *IntLit =<br>
+ dyn_cast<IntegerLiteral>(E->IgnoreParenImpCasts());<br>
+<br>
+ // If initializing from a constant, and the constant starts with '0',<br>
+ // then it is a binary, octal, or hexadecimal. Allow these constants<br>
+ // to fill all the bits, even if there is a sign change.<br>
+ if (!IntLit ||<br>
+ *(S.getSourceManager().getCharacterData(IntLit->getLocStart())) !=<br>
+ '0') {<br>
+<br>
+ std::string PrettySourceValue = Value.toString(10);<br>
+ std::string PrettyTargetValue =<br>
+ PrettyPrintInRange(Value, TargetRange);<br>
+<br>
+ S.DiagRuntimeBehavior(<br>
+ E->getExprLoc(), E,<br>
+ S.PDiag(diag::warn_impcast_integer_precision_constant)<br>
+ << PrettySourceValue << PrettyTargetValue << E->getType() << T<br>
+ << E->getSourceRange() << clang::SourceRange(CC));<br>
+ return;<br>
+ }<br>
+ }<br>
+ }<br>
+ // Fall through for non-constants to give a sign conversion warning.<br>
+ }<br>
+<br>
if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||<br>
(!TargetRange.NonNegative && SourceRange.NonNegative &&<br>
SourceRange.Width == TargetRange.Width)) {<br>
<br>
Modified: cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp?rev=259271&r1=259270&r2=259271&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp?rev=259271&r1=259270&r2=259271&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp (original)<br>
+++ cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp Fri Jan 29 17:51:16 2016<br>
@@ -242,8 +242,8 @@ namespace UndefinedBehavior {<br>
constexpr int n13 = n5 + n5; // expected-error {{constant expression}} expected-note {{value -4294967296 is outside the range of }}<br>
constexpr int n14 = n3 - n5; // expected-error {{constant expression}} expected-note {{value 4294967295 is outside the range of }}<br>
constexpr int n15 = n5 * n5; // expected-error {{constant expression}} expected-note {{value 4611686018427387904 is outside the range of }}<br>
- constexpr signed char c1 = 100 * 2; // ok<br>
- constexpr signed char c2 = '\x64' * '\2'; // also ok<br>
+ constexpr signed char c1 = 100 * 2; // ok expected-warning{{changes value}}<br>
+ constexpr signed char c2 = '\x64' * '\2'; // also ok expected-warning{{changes value}}<br>
constexpr long long ll1 = 0x7fffffffffffffff; // ok<br>
constexpr long long ll2 = ll1 + 1; // expected-error {{constant}} expected-note {{ 9223372036854775808 }}<br>
constexpr long long ll3 = -ll1 - 1; // ok<br>
<br>
Modified: cfe/trunk/test/Sema/constant-conversion.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/constant-conversion.c?rev=259271&r1=259270&r2=259271&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/constant-conversion.c?rev=259271&r1=259270&r2=259271&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Sema/constant-conversion.c (original)<br>
+++ cfe/trunk/test/Sema/constant-conversion.c Fri Jan 29 17:51:16 2016<br>
@@ -80,3 +80,34 @@ void test8() {<br>
struct { enum E x : 1; } f;<br>
f.x = C; // expected-warning {{implicit truncation from 'int' to bitfield changes value from 2 to 0}}<br>
}<br>
+<br>
+void test9() {<br>
+ const char max_char = 0x7F;<br>
+ const short max_short = 0x7FFF;<br>
+ const int max_int = 0x7FFFFFFF;<br>
+<br>
+ const short max_char_plus_one = (short)max_char + 1;<br>
+ const int max_short_plus_one = (int)max_short + 1;<br>
+ const long max_int_plus_one = (long)max_int + 1;<br>
+<br>
+ char new_char = max_char_plus_one; // expected-warning {{implicit conversion from 'const short' to 'char' changes value from 128 to -128}}<br>
+ short new_short = max_short_plus_one; // expected-warning {{implicit conversion from 'const int' to 'short' changes value from 32768 to -32768}}<br>
+ int new_int = max_int_plus_one; // expected-warning {{implicit conversion from 'const long' to 'int' changes value from <a href="tel:2147483648" value="+12147483648" target="_blank">2147483648</a> to -<a href="tel:2147483648" value="+12147483648" target="_blank">2147483648</a>}}<br>
+<br>
+ char hex_char = 0x80;<br>
+ short hex_short = 0x8000;<br>
+ int hex_int = 0x80000000;<br>
+<br>
+ char oct_char = 0200;<br>
+ short oct_short = 0100000;<br>
+ int oct_int = 020000000000;<br>
+<br>
+ char bin_char = 0b10000000;<br>
+ short bin_short = 0b1000000000000000;<br>
+ int bin_int = 0b10000000000000000000000000000000;<br>
+<br>
+#define CHAR_MACRO_HEX 0xff<br>
+ char macro_char_hex = CHAR_MACRO_HEX;<br>
+#define CHAR_MACRO_DEC 255<br>
+ char macro_char_dec = CHAR_MACRO_DEC; // expected-warning {{implicit conversion from 'int' to 'char' changes value from 255 to -1}}<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>