[PATCH] check for Incorrect logic in operator

[Richard Trieu]

A few more comments for you.

+
+#define mydefine 2
+
...
+
+    if ((x < mydefine) <= 10) {}
+} // no-warning

Why have you chosen to ignore warnings on macros?

+std::vector<CFGCallback*> CFGObserver::Observers;
+void CFGObserver::attach(CFGCallback *Observer) {
+  Observers.push_back(Observer);
+}
Add the null pointer check here.  Since the notify functions are called
more often, it makes sense to check for null once here instead of at every
call later.

+void CFGObserver::detach(CFGCallback *Observer) {
+  Observers.erase(std::remove(Observers.begin(), Observers.end(),
Observer),
+                  Observers.end());
+}
Use the one argument erase() here.
+
+void CFGObserver::notifyCompareAlwaysTrue(const BinaryOperator* B,
+                                          bool IsAlwaysTrue) {
+  for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();
+      I != Observers.end(); ++I) {
+    if(*I != 0) {
Remove null check here.
+      (*I)->compareAlwaysTrue(B,IsAlwaysTrue);
+    }
+  }
+}
+
+void CFGObserver::notifyCompareBoolWithInt(const BinaryOperator* B) {
+  for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();
+      I != Observers.end(); ++I) {
+    if(*I != 0) {
And null check here.
+      (*I)->compareBoolWithInt(B);
+    }
+  }
+}
+

+    SourceRange DiagRange(B->getLHS()->getLocStart(),
B->getRHS()->getLocEnd());
+    S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison)
+        << DiagRange << (isAlwaysTrue ? "true" : "false");
+  }
+
We're trying to not pass strings to the diagnostics.  Used
%select{false|true}0 in the .td file then just pass isAlwaysTrue here.

     bool isTemplateInstantiation = false;
     if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D))
       isTemplateInstantiation = Function->isTemplateInstantiation();
-    if (!isTemplateInstantiation)
-      CheckUnreachable(S, AC);
+      if (!isTemplateInstantiation) {
+        LogicalErrorsHandler Leh(S);
+        CFGObserver::attach(&Leh);
+        CheckUnreachable(S, AC);
+        CFGObserver::detach(&Leh);
+      }
   }
I assume this is why -Wunreachable-code is needed in the tests.  This
introduces a dependency which gives different warnings for
-Wtautological-compare depending on an unrelated warning flag.  Consider
running a separate CFG pass for warnings.  Also, this somehow got indented
an extra two spaces.

 def warn_lunsigned_always_true_comparison : Warning<
   "comparison of unsigned%select{| enum}2 expression %0 is always %1">,
   InGroup<TautologicalCompare>;
+def warn_operator_always_true_comparison : Warning<
+  "logical disjunction always evaluates to %0">,
+  InGroup<TautologicalCompare>;
+def warn_bool_and_int_comparison : Warning<
+  "comparison of a boolean expression with an integer other than 0 or 1">,
+  InGroup<TautologicalCompare>;
 def warn_out_of_range_compare : Warning<
   "comparison of constant %0 with expression of type %1 is always "
   "%select{false|true}2">, InGroup<TautologicalOutOfRangeCompare>;
def warn_bool_and_int_comparison should be in TautologicalOutOfRangeCompare
instead of TautologicalCompare.

+/// \brief Keeps track of all observers. Provides functionality to add and
+/// remove observers and notify observers on found logical operator errors.
+class CFGObserver {
+public:
+  static void attach(CFGCallback *Observer);
+  static void detach(CFGCallback *Observer);
+  static void notifyCompareAlwaysTrue(const BinaryOperator* B,
+                                      bool IsAlwaysTrue);
+  static void notifyCompareBoolWithInt(const BinaryOperator* B);
+private:
+  static std::vector<CFGCallback*> Observers;
+};
+
I don't like the idea of essentially a global vector of callbacks for the
CFG.  Perhaps passing a vector to the CFG builder would be a better way.

That's it for the non-CFG code.  I'll see if I can look at that later.

On Mon, Oct 28, 2013 at 1:55 AM, Anders Rönnholm <
Anders.Ronnholm at evidente.se> wrote:

>  Thanks for your comments.****
>
> I was told that it was preferable to update the CFG. The reason is that
> all analyses built on top on the CFG will benefit: -Wuninitialized, static
> analyzer, warnings, etc. I wonder if Anna or Ted could provide their
> opinions about this patch?****
>
> //Anders****
>
> ** **
>
> *From:* Richard Trieu [mailto:rtrieu at google.com]
> *Sent:* den 26 oktober 2013 01:43
> *To:* Jordan Rose
> *Cc:* Anders Rönnholm; cfe-commits at cs.uiuc.edu
> *Subject:* Re: [PATCH] check for Incorrect logic in operator****
>
> ** **
>
> A few high level comments.****
>
> ** **
>
> "-fblocks -Wunreachable-code" is there any reason your test cases have
> these flags?****
>
> "if (!x == 10) {}" there is an existing warning for this called
> -Wlogical-not-parentheses.  The check should be folded into there.****
>
> "if ((x < y) == 10) {}" ditto this and -Wtautological-compare-out-of-range
> ****
>
> "logical disjunction always evaluates to true"  I wouldn't expect all
> programmers to understand what a logical disjunction is.****
>
> ** **
>
> 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?****
>
> ** **
>
> On Fri, Oct 25, 2013 at 10:18 AM, Jordan Rose <jordan_rose at apple.com>
> wrote:****
>
> Richard Trieu has worked on a number of similar warnings. Maybe he can
> take a look?
>
> Jordan****
>
>
>
> On Oct 25, 2013, at 8:18, Anders Rönnholm <Anders.Ronnholm at evidente.se>
> wrote:
>
> > Hi,
> > Resending the patch I sent last week. Anyone want to have a look at it?
> >
> > //Anders
> >
> > -----Original Message-----
> > From: Anders Rönnholm
> > Sent: den 18 oktober 2013 16:05
> > To: cfe-commits at cs.uiuc.edu
> > Subject: [PATCH] check for Incorrect logic in operator
> >
> > Hi,
> >
> > I have a new patch I like to get reviewed. It checks for incorrect
> logics in relational,equal and logic operators in cfgbuilder when trying to
> evaluate bool. A warning is emitted in Sema when an error occurs.
> >
> > eg.
> > if (x != 2 || x != 3) { } always true
> > if ((x < y) <= 10) {}  always true
> >
> > Thanks,
> > Anders****
>
> >
> <incorrectoperatorlogic.diff>_______________________________________________
> > 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/20131029/f6f87ff2/attachment.html>