Re: r259271 - Improve -Wconstant-conversion

[Nico Weber via cfe-commits]

Thanks!

Maybe it's interesting if I list the instances where this fired in Chromium.

* 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.

* Some function taking int was passed 2651229008. I don't understand why;
it was some machine_id thing. The parameter probably wanted to be unsigned.

* In two cases, a char array contained a 255 (this no longer fires after
your change)

* some file had `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).

* finally we had the line `const int16_t kReservedManufacturerID = 1 << 15;`

None of these were bugs, but it seems like a good change anyhow. I can see
how this could catch bugs.


On Fri, Feb 5, 2016 at 6:07 PM, Richard Trieu  wrote:

> r259947 will stop this warning on char arrays.
>
> On Mon, Feb 1, 2016 at 1:40 PM, Richard Trieu  wrote:
>
>> C++11 narrowing only happens during certain conversions while this
>> warning checks all conversions.
>>
>> 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.
>>
>> On Sat, Jan 30, 2016 at 6:41 AM, Nico Weber  wrote:
>>
>>> Shouldn't the new case be in -Wc++11-narrowing instead? This is warning
>>> in similar places where that warning used to warn.
>>>
>>> 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?
>>>
>>> On Fri, Jan 29, 2016 at 6:51 PM, Richard Trieu via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: rtrieu
 Date: Fri Jan 29 17:51:16 2016
 New Revision: 259271

 URL: http://llvm.org/viewvc/llvm-project?rev=259271=rev
 Log:
 Improve -Wconstant-conversion

 Switch the evaluation from isIntegerConstantExpr to EvaluateAsInt.
 EvaluateAsInt will evaluate more types of expressions than
 isIntegerConstantExpr.

 Move one case from -Wsign-conversion to -Wconstant-conversion.  The
 case is:
 1) Source and target types are signed
 2) Source type is wider than the target type
 3) The source constant value is positive
 4) The conversion will store the value as negative in the target.

 Modified:
 cfe/trunk/lib/Sema/SemaChecking.cpp
 cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
 cfe/trunk/test/Sema/constant-conversion.c

 Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271=259270=259271=diff

 ==
 --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 29 17:51:16 2016
 @@ -7578,7 +7578,7 @@ void CheckImplicitConversion(Sema , Ex
  // If the source is a constant, use a default-on diagnostic.
  // TODO: this should happen for bitfield stores, too.
  llvm::APSInt Value(32);
 -if (E->isIntegerConstantExpr(Value, S.Context)) {
 +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects))
 {
if (S.SourceMgr.isInSystemMacro(CC))
  return;

 @@ -7603,6 +7603,42 @@ void CheckImplicitConversion(Sema , Ex
  return DiagnoseImpCast(S, E, T, CC,
 diag::warn_impcast_integer_precision);
}

 +  if (TargetRange.Width == SourceRange.Width &&
 !TargetRange.NonNegative &&
 +  SourceRange.NonNegative && Source->isSignedIntegerType()) {
 +// Warn when doing a signed to signed conversion, warn if the
 positive
 +// source value is exactly the width of the target type, which will
 +// cause a negative value to be stored.
 +
 +llvm::APSInt Value;
 +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects))
 {
 +  if (!S.SourceMgr.isInSystemMacro(CC)) {
 +
 +IntegerLiteral *IntLit =
 +dyn_cast(E->IgnoreParenImpCasts());
 +
 +// If initializing from a constant, and the constant starts
 with '0',
 +// then it is a binary, octal, or hexadecimal.  Allow 

Re: r259271 - Improve -Wconstant-conversion

[Richard Trieu via cfe-commits]

r259947 will stop this warning on char arrays.

On Mon, Feb 1, 2016 at 1:40 PM, Richard Trieu  wrote:

> C++11 narrowing only happens during certain conversions while this warning
> checks all conversions.
>
> 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.
>
> On Sat, Jan 30, 2016 at 6:41 AM, Nico Weber  wrote:
>
>> Shouldn't the new case be in -Wc++11-narrowing instead? This is warning
>> in similar places where that warning used to warn.
>>
>> 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?
>>
>> On Fri, Jan 29, 2016 at 6:51 PM, Richard Trieu via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: rtrieu
>>> Date: Fri Jan 29 17:51:16 2016
>>> New Revision: 259271
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259271=rev
>>> Log:
>>> Improve -Wconstant-conversion
>>>
>>> Switch the evaluation from isIntegerConstantExpr to EvaluateAsInt.
>>> EvaluateAsInt will evaluate more types of expressions than
>>> isIntegerConstantExpr.
>>>
>>> Move one case from -Wsign-conversion to -Wconstant-conversion.  The case
>>> is:
>>> 1) Source and target types are signed
>>> 2) Source type is wider than the target type
>>> 3) The source constant value is positive
>>> 4) The conversion will store the value as negative in the target.
>>>
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
>>> cfe/trunk/test/Sema/constant-conversion.c
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271=259270=259271=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 29 17:51:16 2016
>>> @@ -7578,7 +7578,7 @@ void CheckImplicitConversion(Sema , Ex
>>>  // If the source is a constant, use a default-on diagnostic.
>>>  // TODO: this should happen for bitfield stores, too.
>>>  llvm::APSInt Value(32);
>>> -if (E->isIntegerConstantExpr(Value, S.Context)) {
>>> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
>>>if (S.SourceMgr.isInSystemMacro(CC))
>>>  return;
>>>
>>> @@ -7603,6 +7603,42 @@ void CheckImplicitConversion(Sema , Ex
>>>  return DiagnoseImpCast(S, E, T, CC,
>>> diag::warn_impcast_integer_precision);
>>>}
>>>
>>> +  if (TargetRange.Width == SourceRange.Width &&
>>> !TargetRange.NonNegative &&
>>> +  SourceRange.NonNegative && Source->isSignedIntegerType()) {
>>> +// Warn when doing a signed to signed conversion, warn if the
>>> positive
>>> +// source value is exactly the width of the target type, which will
>>> +// cause a negative value to be stored.
>>> +
>>> +llvm::APSInt Value;
>>> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
>>> +  if (!S.SourceMgr.isInSystemMacro(CC)) {
>>> +
>>> +IntegerLiteral *IntLit =
>>> +dyn_cast(E->IgnoreParenImpCasts());
>>> +
>>> +// If initializing from a constant, and the constant starts
>>> with '0',
>>> +// then it is a binary, octal, or hexadecimal.  Allow these
>>> constants
>>> +// to fill all the bits, even if there is a sign change.
>>> +if (!IntLit ||
>>> +
>>> *(S.getSourceManager().getCharacterData(IntLit->getLocStart())) !=
>>> +'0') {
>>> +
>>> +  std::string PrettySourceValue = Value.toString(10);
>>> +  std::string PrettyTargetValue =
>>> +  PrettyPrintInRange(Value, TargetRange);
>>> +
>>> +  S.DiagRuntimeBehavior(
>>> +  E->getExprLoc(), E,
>>> +  S.PDiag(diag::warn_impcast_integer_precision_constant)
>>> +  << PrettySourceValue << PrettyTargetValue <<
>>> E->getType() << T
>>> +  << E->getSourceRange() << clang::SourceRange(CC));
>>> +  return;
>>> +}
>>> +  }
>>> +}
>>> +// Fall through for non-constants to give a sign conversion warning.
>>> +  }
>>> +
>>>if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||
>>>(!TargetRange.NonNegative && SourceRange.NonNegative &&
>>> SourceRange.Width == TargetRange.Width)) {
>>>
>>> Modified: cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
>>> URL:
>>> 

Re: r259271 - Improve -Wconstant-conversion

[Richard Trieu via cfe-commits]

C++11 narrowing only happens during certain conversions while this warning
checks all conversions.

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.

On Sat, Jan 30, 2016 at 6:41 AM, Nico Weber  wrote:

> Shouldn't the new case be in -Wc++11-narrowing instead? This is warning in
> similar places where that warning used to warn.
>
> 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?
>
> On Fri, Jan 29, 2016 at 6:51 PM, Richard Trieu via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rtrieu
>> Date: Fri Jan 29 17:51:16 2016
>> New Revision: 259271
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=259271=rev
>> Log:
>> Improve -Wconstant-conversion
>>
>> Switch the evaluation from isIntegerConstantExpr to EvaluateAsInt.
>> EvaluateAsInt will evaluate more types of expressions than
>> isIntegerConstantExpr.
>>
>> Move one case from -Wsign-conversion to -Wconstant-conversion.  The case
>> is:
>> 1) Source and target types are signed
>> 2) Source type is wider than the target type
>> 3) The source constant value is positive
>> 4) The conversion will store the value as negative in the target.
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
>> cfe/trunk/test/Sema/constant-conversion.c
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271=259270=259271=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 29 17:51:16 2016
>> @@ -7578,7 +7578,7 @@ void CheckImplicitConversion(Sema , Ex
>>  // If the source is a constant, use a default-on diagnostic.
>>  // TODO: this should happen for bitfield stores, too.
>>  llvm::APSInt Value(32);
>> -if (E->isIntegerConstantExpr(Value, S.Context)) {
>> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
>>if (S.SourceMgr.isInSystemMacro(CC))
>>  return;
>>
>> @@ -7603,6 +7603,42 @@ void CheckImplicitConversion(Sema , Ex
>>  return DiagnoseImpCast(S, E, T, CC,
>> diag::warn_impcast_integer_precision);
>>}
>>
>> +  if (TargetRange.Width == SourceRange.Width && !TargetRange.NonNegative
>> &&
>> +  SourceRange.NonNegative && Source->isSignedIntegerType()) {
>> +// Warn when doing a signed to signed conversion, warn if the
>> positive
>> +// source value is exactly the width of the target type, which will
>> +// cause a negative value to be stored.
>> +
>> +llvm::APSInt Value;
>> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
>> +  if (!S.SourceMgr.isInSystemMacro(CC)) {
>> +
>> +IntegerLiteral *IntLit =
>> +dyn_cast(E->IgnoreParenImpCasts());
>> +
>> +// If initializing from a constant, and the constant starts with
>> '0',
>> +// then it is a binary, octal, or hexadecimal.  Allow these
>> constants
>> +// to fill all the bits, even if there is a sign change.
>> +if (!IntLit ||
>> +
>> *(S.getSourceManager().getCharacterData(IntLit->getLocStart())) !=
>> +'0') {
>> +
>> +  std::string PrettySourceValue = Value.toString(10);
>> +  std::string PrettyTargetValue =
>> +  PrettyPrintInRange(Value, TargetRange);
>> +
>> +  S.DiagRuntimeBehavior(
>> +  E->getExprLoc(), E,
>> +  S.PDiag(diag::warn_impcast_integer_precision_constant)
>> +  << PrettySourceValue << PrettyTargetValue <<
>> E->getType() << T
>> +  << E->getSourceRange() << clang::SourceRange(CC));
>> +  return;
>> +}
>> +  }
>> +}
>> +// Fall through for non-constants to give a sign conversion warning.
>> +  }
>> +
>>if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||
>>(!TargetRange.NonNegative && SourceRange.NonNegative &&
>> SourceRange.Width == TargetRange.Width)) {
>>
>> Modified: cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp?rev=259271=259270=259271=diff
>>
>> ==
>> --- cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp (original)
>> +++ 

Re: r259271 - Improve -Wconstant-conversion

[Nico Weber via cfe-commits]

Shouldn't the new case be in -Wc++11-narrowing instead? This is warning in
similar places where that warning used to warn.

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?

On Fri, Jan 29, 2016 at 6:51 PM, Richard Trieu via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rtrieu
> Date: Fri Jan 29 17:51:16 2016
> New Revision: 259271
>
> URL: http://llvm.org/viewvc/llvm-project?rev=259271=rev
> Log:
> Improve -Wconstant-conversion
>
> Switch the evaluation from isIntegerConstantExpr to EvaluateAsInt.
> EvaluateAsInt will evaluate more types of expressions than
> isIntegerConstantExpr.
>
> Move one case from -Wsign-conversion to -Wconstant-conversion.  The case
> is:
> 1) Source and target types are signed
> 2) Source type is wider than the target type
> 3) The source constant value is positive
> 4) The conversion will store the value as negative in the target.
>
> Modified:
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
> cfe/trunk/test/Sema/constant-conversion.c
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=259271=259270=259271=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 29 17:51:16 2016
> @@ -7578,7 +7578,7 @@ void CheckImplicitConversion(Sema , Ex
>  // If the source is a constant, use a default-on diagnostic.
>  // TODO: this should happen for bitfield stores, too.
>  llvm::APSInt Value(32);
> -if (E->isIntegerConstantExpr(Value, S.Context)) {
> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
>if (S.SourceMgr.isInSystemMacro(CC))
>  return;
>
> @@ -7603,6 +7603,42 @@ void CheckImplicitConversion(Sema , Ex
>  return DiagnoseImpCast(S, E, T, CC,
> diag::warn_impcast_integer_precision);
>}
>
> +  if (TargetRange.Width == SourceRange.Width && !TargetRange.NonNegative
> &&
> +  SourceRange.NonNegative && Source->isSignedIntegerType()) {
> +// Warn when doing a signed to signed conversion, warn if the positive
> +// source value is exactly the width of the target type, which will
> +// cause a negative value to be stored.
> +
> +llvm::APSInt Value;
> +if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {
> +  if (!S.SourceMgr.isInSystemMacro(CC)) {
> +
> +IntegerLiteral *IntLit =
> +dyn_cast(E->IgnoreParenImpCasts());
> +
> +// If initializing from a constant, and the constant starts with
> '0',
> +// then it is a binary, octal, or hexadecimal.  Allow these
> constants
> +// to fill all the bits, even if there is a sign change.
> +if (!IntLit ||
> +
> *(S.getSourceManager().getCharacterData(IntLit->getLocStart())) !=
> +'0') {
> +
> +  std::string PrettySourceValue = Value.toString(10);
> +  std::string PrettyTargetValue =
> +  PrettyPrintInRange(Value, TargetRange);
> +
> +  S.DiagRuntimeBehavior(
> +  E->getExprLoc(), E,
> +  S.PDiag(diag::warn_impcast_integer_precision_constant)
> +  << PrettySourceValue << PrettyTargetValue <<
> E->getType() << T
> +  << E->getSourceRange() << clang::SourceRange(CC));
> +  return;
> +}
> +  }
> +}
> +// Fall through for non-constants to give a sign conversion warning.
> +  }
> +
>if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||
>(!TargetRange.NonNegative && SourceRange.NonNegative &&
> SourceRange.Width == TargetRange.Width)) {
>
> Modified: cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp?rev=259271=259270=259271=diff
>
> ==
> --- cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp (original)
> +++ cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp Fri Jan 29 17:51:16 2016
> @@ -242,8 +242,8 @@ namespace UndefinedBehavior {
>  constexpr int n13 = n5 + n5; // expected-error {{constant
> expression}} expected-note {{value -4294967296 is outside the range of }}
>  constexpr int n14 = n3 - n5; // expected-error {{constant
> expression}} expected-note {{value 4294967295 is outside the range of }}
>  constexpr int n15 = n5 * n5; // expected-error {{constant
> expression}} expected-note {{value 4611686018427387904 is outside the range
> of }}
> -constexpr signed