Fixed/improved loplugin:cppunitassertequals

* 994e38e336beeacbd983faafac480afc94d3947e "loplugin: use TypeCheck instead of
  getQualifiedNameAsString" had effectively disabled this plugin (Asserter is a
  struct, not a namespace).  Fixed that.

* Also improved the checks, for one removing the---expensive---use of
  Plugin::parentStmt, for another making the plugin look into (...), !..., and
  ...&&... expressions.

* However, as the plugin had effectively already been disabled (see above) when
  it was switched on generally with 839e53b933322b739a7f534af58c63a2c69af7bd
  "compilerplugins: enable loplugin:cppunitassertequals by default", it now hit
  way more places than I had initially anticipated.  To keep the amount of work
  manageable, midway-through I disabled looking into ...&&... expressions for
  now.  That will be enabled (and resulting warnings fixed) in follow-up
  commits.

* Checks like

    CPPUNIT_ASSERT(a == b)

  that actually want to check a specific overloaded operator == implementation,
  rather than using such an operator == to actually check that a and b are
  equal, can be rewritten as

    CPPUNIT_ASSERT(operator ==(a, b))

  to avoid false warnings from this plugin.

Change-Id: If3501020e2d150ad0f2454a65a39081e31470c0f

1 files changed, 98 insertions, 44 deletions

diff --git a/compilerplugins/clang/cppunitassertequals.cxx b/compilerplugins/clang/cppunitassertequals.cxx
index 858ad9615cbe..37dff6acca97 100644
--- a/compilerplugins/clang/cppunitassertequals.cxx
+++ b/compilerplugins/clang/cppunitassertequals.cxx
@@ -7,11 +7,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-#include <cassert>
-#include <string>
-#include <iostream>
-#include <fstream>
-#include <set>
 #include "plugin.hxx"
 #include "check.hxx"
 
@@ -29,74 +24,133 @@ public:
 
     virtual void run() override
     {
-        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        if (compiler.getLangOpts().CPlusPlus) {
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        }
     }
 
     bool VisitCallExpr(const CallExpr*);
-    bool VisitBinaryOperator(const BinaryOperator*);
+
 private:
-    void checkExpr(const Stmt* stmt);
+    void checkExpr(
+        SourceRange range, StringRef name, Expr const * expr, bool negated);
+
+    void reportEquals(SourceRange range, StringRef name, bool negative);
 };
 
 bool CppunitAssertEquals::VisitCallExpr(const CallExpr* callExpr)
 {
-    if (ignoreLocation(callExpr)) {
+    auto const decl = callExpr->getDirectCallee();
+    if (decl == nullptr
+        || !(loplugin::DeclCheck(decl).Function("failIf").Struct("Asserter")
+             .Namespace("CppUnit").GlobalNamespace()))
+    {
         return true;
     }
-    if (callExpr->getDirectCallee() == nullptr) {
+    // Don't use callExpr->getLocStart() or callExpr->getExprLoc(), as those
+    // fall into a nested use of the CPPUNIT_NS macro; CallExpr::getRParenLoc
+    // happens to be readily available and cause good results:
+    auto loc = callExpr->getRParenLoc();
+    while (compiler.getSourceManager().isMacroArgExpansion(loc)) {
+        loc = compiler.getSourceManager().getImmediateMacroCallerLoc(loc);
+    }
+    if (!compiler.getSourceManager().isMacroBodyExpansion(loc)
+        || ignoreLocation(
+            compiler.getSourceManager().getImmediateMacroCallerLoc(loc)))
+    {
         return true;
     }
-    const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
-    if (functionDecl->getOverloadedOperator() != OO_EqualEqual) {
+    auto name = Lexer::getImmediateMacroName(
+        loc, compiler.getSourceManager(), compiler.getLangOpts());
+    if (name != "CPPUNIT_ASSERT" && name != "CPPUNIT_ASSERT_MESSAGE") {
         return true;
     }
-    checkExpr(callExpr);
-    return true;
-}
-
-bool CppunitAssertEquals::VisitBinaryOperator(const BinaryOperator* binaryOp)
-{
-    if (ignoreLocation(binaryOp)) {
+    if (decl->getNumParams() != 3) {
+        report(
+            DiagnosticsEngine::Warning,
+            ("TODO: suspicious CppUnit::Asserter::failIf call with %0"
+             " parameters"),
+            callExpr->getExprLoc())
+            << decl->getNumParams() << callExpr->getSourceRange();
         return true;
     }
-    if (binaryOp->getOpcode() != BO_EQ) {
+    auto const e1 = callExpr->getArg(0)->IgnoreParenImpCasts();
+    Expr const * e2 = nullptr;
+    if (auto const e3 = dyn_cast<UnaryOperator>(e1)) {
+        if (e3->getOpcode() == UO_LNot) {
+            e2 = e3->getSubExpr();
+        }
+    } else if (auto const e4 = dyn_cast<CXXOperatorCallExpr>(e1)) {
+        if (e4->getOperator() == OO_Exclaim) {
+            e2 = e4->getArg(0);
+        }
+    }
+    if (e2 == nullptr) {
+        report(
+            DiagnosticsEngine::Warning,
+            ("TODO: suspicious CppUnit::Asserter::failIf call not wrapping"
+             " !(...)"),
+            callExpr->getExprLoc())
+            << callExpr->getSourceRange();
         return true;
     }
-    checkExpr(binaryOp);
+    auto range = compiler.getSourceManager().getImmediateExpansionRange(loc);
+    checkExpr(
+        SourceRange(range.first, range.second), name,
+        e2->IgnoreParenImpCasts(), false);
     return true;
 }
 
-/*
- * check that we are not a compound expression
- */
-void CppunitAssertEquals::checkExpr(const Stmt* stmt)
+void CppunitAssertEquals::checkExpr(
+    SourceRange range, StringRef name, Expr const * expr, bool negated)
 {
-    const Stmt* parent = parentStmt(stmt);
-    if (!parent || !isa<ParenExpr>(parent)) {
-        return;
-    }
-    parent = parentStmt(parent);
-    if (!parent || !isa<UnaryOperator>(parent)) {
-        return;
-    }
-    parent = parentStmt(parent);
-    if (!parent || !isa<CallExpr>(parent)) {
+    if (auto const e = dyn_cast<UnaryOperator>(expr)) {
+        if (e->getOpcode() == UO_LNot) {
+            checkExpr(
+                range, name, e->getSubExpr()->IgnoreParenImpCasts(), !negated);
+        }
         return;
     }
-    const CallExpr* callExpr = dyn_cast<CallExpr>(parent);
-    if (!callExpr || callExpr->getDirectCallee() == nullptr) {
+    if (auto const e = dyn_cast<BinaryOperator>(expr)) {
+        auto const op = e->getOpcode();
+        if ((!negated && op == BO_EQ) || (negated && op == BO_NE)) {
+            reportEquals(range, name, op == BO_NE);
+            return;
+        }
+#if 0 // TODO: enable later
+        if ((!negated && op == BO_LAnd) || (negated && op == BO_LOr)) {
+            report(
+                DiagnosticsEngine::Warning,
+                "rather split into two %0", e->getExprLoc())
+                << name << range;
+            return;
+        }
+#endif
         return;
     }
-    const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
-    if (!functionDecl ||
-        !loplugin::DeclCheck(functionDecl).Function("failIf").Namespace("Asserter").Namespace("CppUnit").GlobalNamespace())
-    {
+    if (auto const e = dyn_cast<CXXOperatorCallExpr>(expr)) {
+        auto const op = e->getOperator();
+        if ((!negated && op == OO_EqualEqual)
+            || (negated && op == OO_ExclaimEqual))
+        {
+            reportEquals(range, name, op == OO_ExclaimEqual);
+            return;
+        }
         return;
     }
+}
+
+void CppunitAssertEquals::reportEquals(
+    SourceRange range, StringRef name, bool negative)
+{
     report(
-        DiagnosticsEngine::Warning, "rather call CPPUNIT_ASSERT_EQUALS",
-        stmt->getSourceRange().getBegin())
-      << stmt->getSourceRange();
+        DiagnosticsEngine::Warning,
+        ("rather call"
+         " %select{CPPUNIT_ASSERT_EQUAL|CPPUNIT_ASSERT_EQUAL_MESSAGE}0 (or"
+         " rewrite as an explicit operator %select{==|!=}1 call when the"
+         " operator itself is the topic)"),
+        range.getBegin())
+        << (name == "CPPUNIT_ASSERT_MESSAGE") << negative << range;
 }
 
 loplugin::Plugin::Registration< CppunitAssertEquals > X("cppunitassertequals");