[PATCH] Warn on suspicious use of absolute value function

[Richard Trieu]

Hi Jay,

This warning makes a few assumptions about your code:
1) Using an unsigned value means you only want non-negative values
2) You expect the absolute value function to be as perfect as possible

With both assumptions, unsigned values are already non-negative, so the
best absolute value function would simply return the input value.  Thus,
Clang suggests removing the call to abs().

>From your example, since unsigned and int are the same size, conversion
between unsigned and int will give you the correct difference if the
difference is less than INT_MAX.  Above that and you have wrapping issues.
The best way to get the difference is:

unsigned f(unsigned a, unsigned b) {
  if (a > b)
    return a - b;
  else
    return b - a;
}

This works for any pair of unsigned values.

Richard

On Tue, Oct 7, 2014 at 3:47 AM, Jay Foad <jay.foad at gmail.com> wrote:

> Hi Richard,
>
> I get:
>
> $ cat a.c
> #include <stdlib.h>
> int f(unsigned x, unsigned y) { return abs(x - y); }
> $ ./clang -fsyntax-only a.c
> a.c:2:40: warning: taking the absolute value of unsigned type
> 'unsigned int' has no effect [-Wabsolute-value]
> int f(unsigned x, unsigned y) { return abs(x - y); }
> [...]
>
> Is it really right that the call to abs "has no effect"? Surely it
> does have an effect: the unsigned value x-y is converted to int, and
> if that value is negative then abs() will negate it.
>
> Or have I misunderstood C?
>
> Thanks,
> Jay.
>
> On 20 November 2013 02:08, Richard Trieu <rtrieu at google.com> wrote:
> > This is a new warning to detect improper uses of the absolute value
> function.  Three new warnings are introduced with this patch.
> >
> > 1) Attempting to take the absolute value of an unsigned integer.
> Includes fix-it to remove the absolute value function call.
> > 2) Using an absolute value function that is too small.  For instance,
> using abs(int) on a long would give a warning and a fix-it to use labs.
> > 3) Using the wrong type of absolute value (integer, floating point,
> complex) for the argument given.  Includes a fix-it to the proper absolute
> value type.
> >
> > I have already run this over a large code base and found hundreds of
> questionable usages of absolute value.
> >
> > http://llvm-reviews.chandlerc.com/D2224
> >
> > Files:
> >   test/Sema/warn-absolute-value.c
> >   include/clang/Basic/DiagnosticSemaKinds.td
> >   include/clang/Basic/DiagnosticGroups.td
> >   include/clang/Sema/Sema.h
> >   include/clang/AST/Decl.h
> >   lib/Sema/SemaChecking.cpp
> >   lib/AST/Decl.cpp
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141007/e770076d/attachment.html>