[PATCH] check for Incorrect logic in operator

[Richard Trieu]

On Mon, Oct 28, 2013 at 11:47 AM, Ted Kremenek <kremenek at apple.com> wrote:

> On Oct 28, 2013, at 11:41 AM, Ted Kremenek <kremenek at apple.com> wrote:
>
> On Oct 25, 2013, at 4:42 PM, Richard Trieu <rtrieu at google.com> wrote:
>
> If you check out -Wlogical-not-parentheses and
> -Wtautological-compare-out-of-range, they are in lib/Sema/SemaChecking.cpp.
>  Why did you use a CFG approach instead?
>
>
> Hi Richard,
>
> I told Anders that modifying the CFG builder would be an interesting
> approach for two reasons:
>
> (1) The CFG is used for a variety of dataflow analyses, thus folding this
> “smarts” into the CFG builder seems to have dividends for other uses.
>
> (2) The CFG builder already does some branch pruning by doing some simply
> analysis of expressions at branches.  This seemed like a natural
> enhancement.
>
> I did consider this to take a similar approach to
> -Wlogical-not-parentheses, but thought this would be a more promising
> direction to explore.
>
> Flipping the question around, do you think it would be interesting to
> consider the opposite approach and move the logic to
> -Wlogical-not-parentheses, etc., into the CFG builder, for the same reasons
> I have given above?
>
> Ted
>
>
> Further, there is a very natural relationship between *all* of these
> warnings and -Wunreachable-code (a warning we’d one day like to fully
> productize), which is built directly on top of the CFG.  Bringing the
> underlying pieces closer together seems like a promising direction to
> explore.
>

My worry is that using the CFG won't check all the code a Sema based
warning would (ignoring intentionally skipped dead/unreachable code).  This
would then require code in both the CFG and Sema for a warning to cover all
cases.  Clang already has this for the self-reference on initialization
warning.  If you have "int x = x + x;" in a function scope, the CFG-based
uninitialized checker will produce the warning.  If that same code is in a
global scope, the AST-based check is performed in Sema.  I think it's
better to have the warning in one place, but that may differ by warning.

Some warnings might benefit having its results be fed back into CFG, such
as using -Wtautological-compare for branch analysis, although the enum part
might need to be changed.  Other warnings, such as warnings based on size
would be good to silence on unreachable code.  On the other hand,
-Wlogical-not-parentheses doesn't provide any information to the CFG, and
is almost always a true positive so warning on unreachable code is
desirable.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131028/34b8cbf3/attachment.html>