[Sema] Don't warn if taking the absolute value of a difference of unsigned values.

[cdavis5x]

Summary

While any given unsigned value usually cannot be negative, a difference
of two unsigned values can be negative iff the subtrahend (i.e. the
second value) is greater than the minuend. For this reason, it is common
to wrap a difference like this in an abs(3) call. Don't warn on these
cases.

[craig.topper]

I think you CCed the wrong mailing list. I think you want cfe-commits.

[cdavis5x]

Oops, you're right. Now I've added the correct list, but how do I get llvm-commits unsubscribed?

[rsmith]

The difference of two unsigned promoted types is also unsigned, so cannot be negative. Perhaps we should word the warning differently in this case, but a warning does seem justified.

lib/Sema/SemaChecking.cpp

void Sema::CheckAbsoluteValueFunction(const CallExpr *Call,
   if (Call->getNumArgs() != 1)
     return;
 
   unsigned AbsKind = getAbsoluteValueFunctionKind(FDecl);
   bool IsStdAbs = IsFunctionStdAbs(FDecl);
   if (AbsKind == 0 && !IsStdAbs)
     return;
 
-  QualType ArgType = Call->getArg(0)->IgnoreParenImpCasts()->getType();
+  const Expr *ArgExpr = Call->getArg(0)->IgnoreParenImpCasts();
+  QualType ArgType = ArgExpr->getType();
   QualType ParamType = Call->getArg(0)->getType();
 
-  // Unsigned types cannot be negative.  Suggest removing the absolute value
+  // Unsigned types cannot be negative, but a difference of two unsigned
+  // values can be negative if the subtrahend is greater.  If the
+  // expression is not a difference, suggest removing the absolute value
   // function call.
   if (ArgType->isUnsignedIntegerType()) {
+    if (!isa<BinaryOperator>(ArgExpr) ||
+        cast<BinaryOperator>(ArgExpr)->getOpcode() != BO_Sub) {
       const char *FunctionName =
           IsStdAbs ? "std::abs" : Context.BuiltinInfo.GetName(AbsKind);
       Diag(Call->getExprLoc(), diag::warn_unsigned_abs) << ArgType << ParamType;
       Diag(Call->getExprLoc(), diag::note_remove_abs)
           << FunctionName
           << FixItHint::CreateRemoval(Call->getCallee()->getSourceRange());
       return;
     }
+    // Assume the argument type is the corresponding signed type for the
+    // rest of this analysis.
+    if (ArgType->isSpecificBuiltinType(BuiltinType::ULong))
+      ArgType = Context.LongTy;
+    else if (ArgType->isSpecificBuiltinType(BuiltinType::ULongLong))
+      ArgType = Context.LongLongTy;
+    else
+      ArgType = Context.IntTy;
+  }
 
   // std::abs has overloads which prevent most of the absolute value problems
   // from occurring.
   if (IsStdAbs)
     return;
 
   AbsoluteValueKind ArgValueKind = getAbsoluteValueKind(ArgType);
   AbsoluteValueKind ParamValueKind = getAbsoluteValueKind(ParamType);

test/Sema/warn-absolute-value.c

void test_unsigned_long(unsigned long x) {
   // expected-note@-2 {{remove the call to '__builtin_cabs' since unsigned values cannot be negative}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:""
   (void)__builtin_cabsl(x);
   // expected-warning@-1 {{taking the absolute value of unsigned type 'unsigned long' has no effect}}
   // expected-note@-2 {{remove the call to '__builtin_cabsl' since unsigned values cannot be negative}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:""
 }
 
+void test_unsigned_int_difference(unsigned int x, unsigned int y) {
+  (void)abs(x-y);
+  (void)labs(x-y);
+  (void)llabs(x-y);
+
+  (void)fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"abs"
+  (void)fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"abs"
+  (void)fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"abs"
+
+  (void)cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"abs"
+  (void)cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"abs"
+  (void)cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"abs"
+
+  (void)__builtin_abs(x-y);
+  (void)__builtin_labs(x-y);
+  (void)__builtin_llabs(x-y);
+
+  (void)__builtin_fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_abs"
+  (void)__builtin_fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"__builtin_abs"
+  (void)__builtin_fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_abs"
+
+  (void)__builtin_cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_abs"
+  (void)__builtin_cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"__builtin_abs"
+  (void)__builtin_cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_abs"
+}
+
+void test_unsigned_long_difference(unsigned long x, unsigned long y) {
+  (void)abs(x-y);  // no warning - int and long are same length for this target
+  (void)labs(x-y);
+  (void)llabs(x-y);
+
+  (void)fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"labs"
+  (void)fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"labs"
+  (void)fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"labs"
+
+  (void)cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"labs"
+  (void)cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"labs"
+  (void)cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"labs"
+
+  (void)__builtin_abs(x-y);  // no warning - int and long are same length for
+                             // this target
+  (void)__builtin_labs(x-y);
+  (void)__builtin_llabs(x-y);
+
+  (void)__builtin_fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_labs"
+  (void)__builtin_fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"__builtin_labs"
+  (void)__builtin_fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_labs"
+
+  (void)__builtin_cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_labs"
+  (void)__builtin_cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"__builtin_labs"
+  (void)__builtin_cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function '__builtin_labs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"__builtin_labs"
+}
+

test/SemaCXX/warn-absolute-value.cpp

void test_unsigned_long(unsigned long x) {
   // expected-note@-2 {{remove the call to '__builtin_cabs' since unsigned values cannot be negative}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:""
   (void)__builtin_cabsl(x);
   // expected-warning@-1 {{taking the absolute value of unsigned type 'unsigned long' has no effect}}
   // expected-note@-2 {{remove the call to '__builtin_cabsl' since unsigned values cannot be negative}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:""
 }
 
+void test_unsigned_int_difference(unsigned int x, unsigned int y) {
+  (void)std::abs(x-y);
+
+  (void)abs(x-y);
+  (void)labs(x-y);
+  (void)llabs(x-y);
+
+  (void)fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+  (void)fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"std::abs"
+  (void)fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+
+  (void)cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+  (void)cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"std::abs"
+  (void)cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+
+  (void)__builtin_abs(x-y);
+  (void)__builtin_labs(x-y);
+  (void)__builtin_llabs(x-y);
+
+  (void)__builtin_fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+  (void)__builtin_fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"std::abs"
+  (void)__builtin_fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+
+  (void)__builtin_cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+  (void)__builtin_cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"std::abs"
+  (void)__builtin_cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+}
+
+void test_unsigned_long_difference(unsigned long x, unsigned long y) {
+  (void)std::abs(x-y);
+
+  (void)abs(x-y);  // no warning - int and long are same length for this target
+  (void)labs(x-y);
+  (void)llabs(x-y);
+
+  (void)fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+  (void)fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"std::abs"
+  (void)fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function 'fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+
+  (void)cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+  (void)cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:13}:"std::abs"
+  (void)cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function 'cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:14}:"std::abs"
+
+  (void)__builtin_abs(x-y);  // no warning - int and long are same length for
+                             // this target
+  (void)__builtin_labs(x-y);
+  (void)__builtin_llabs(x-y);
+
+  (void)__builtin_fabsf(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+  (void)__builtin_fabs(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"std::abs"
+  (void)__builtin_fabsl(x-y);
+  // expected-warning@-1 {{using floating point absolute value function '__builtin_fabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+
+  (void)__builtin_cabsf(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsf' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+  (void)__builtin_cabs(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabs' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:23}:"std::abs"
+  (void)__builtin_cabsl(x-y);
+  // expected-warning@-1 {{using complex absolute value function '__builtin_cabsl' when argument is of integer type}}
+  // expected-note@-2 {{use function 'std::abs' instead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:9-[[@LINE-3]]:24}:"std::abs"
+}
+