look for =() in loplugin:unnecessaryparen

Change-Id: I4f9b71ff7767e90987bb40358fc46ed5d1d571d0
Reviewed-on: https://gerrit.libreoffice.org/44944
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 101 insertions, 17 deletions

diff --git a/compilerplugins/clang/test/unnecessaryparen.cxx b/compilerplugins/clang/test/unnecessaryparen.cxx
index 51f792769bc2..d07f4930c4d3 100644
--- a/compilerplugins/clang/test/unnecessaryparen.cxx
+++ b/compilerplugins/clang/test/unnecessaryparen.cxx
@@ -7,6 +7,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+#include <string>
+#include <rtl/ustring.hxx>
+
 bool foo(int);
 
 enum class EFoo { Bar };
@@ -20,7 +23,7 @@ int main()
 
     foo((1)); // expected-error {{parentheses immediately inside single-arg call [loplugin:unnecessaryparen]}}
 
-    int y = (x); // expected-error {{unnecessary parentheses around identifier [loplugin:unnecessaryparen]}}
+    int y = (x); // expected-error {{unnecessary parentheses around identifier [loplugin:unnecessaryparen]}} expected-error {{parentheses immediately inside vardecl statement [loplugin:unnecessaryparen]}}
     (void)y;
 
     EFoo foo = EFoo::Bar;
@@ -40,6 +43,13 @@ int main()
         return 0;
     }
     x = (true) ? 0 : 1;
+
+    int v2 = (1); // expected-error {{parentheses immediately inside vardecl statement [loplugin:unnecessaryparen]}}
+    (void)v2;
+
+    std::string v3;
+    v3 = (std::string("xx") + "xx"); // expected-error {{parentheses immediately inside assignment [loplugin:unnecessaryparen]}}
+    (void)v3;
 };
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unnecessaryparen.cxx b/compilerplugins/clang/unnecessaryparen.cxx
index 43fc13be8eea..57c9cd9c65b4 100644
--- a/compilerplugins/clang/unnecessaryparen.cxx
+++ b/compilerplugins/clang/unnecessaryparen.cxx
@@ -79,11 +79,13 @@ public:
     bool VisitCaseStmt(const CaseStmt *);
     bool VisitReturnStmt(const ReturnStmt* );
     bool VisitCallExpr(const CallExpr *);
+    bool VisitVarDecl(const VarDecl *);
+    bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *);
     bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *);
     bool TraverseCaseStmt(CaseStmt *);
     bool TraverseConditionalOperator(ConditionalOperator *);
 private:
-    void VisitSomeStmt(const Stmt *parent, const Expr* cond, StringRef stmtName);
+    void VisitSomeStmt(Stmt const * stmt, const Expr* cond, StringRef stmtName);
     Expr const * insideSizeof = nullptr;
     Expr const * insideCaseStmt = nullptr;
     Expr const * insideConditionalOperator = nullptr;
@@ -234,11 +236,11 @@ bool UnnecessaryParen::VisitReturnStmt(const ReturnStmt* returnStmt)
     return true;
 }
 
-void UnnecessaryParen::VisitSomeStmt(const Stmt *parent, const Expr* cond, StringRef stmtName)
+void UnnecessaryParen::VisitSomeStmt(const Stmt * stmt, const Expr* cond, StringRef stmtName)
 {
-    if (ignoreLocation(parent))
+    if (ignoreLocation(stmt))
         return;
-    if (parent->getLocStart().isMacroID())
+    if (stmt->getLocStart().isMacroID())
         return;
 
     auto parenExpr = dyn_cast<ParenExpr>(ignoreAllImplicit(cond));
@@ -273,18 +275,90 @@ bool UnnecessaryParen::VisitCallExpr(const CallExpr* callExpr)
         return true;
 
     auto parenExpr = dyn_cast<ParenExpr>(ignoreAllImplicit(callExpr->getArg(0)));
-    if (parenExpr) {
-        if (parenExpr->getLocStart().isMacroID())
-            return true;
-        // assignments need extra parentheses or they generate a compiler warning
-        auto binaryOp = dyn_cast<BinaryOperator>(parenExpr->getSubExpr());
-        if (binaryOp && binaryOp->getOpcode() == BO_Assign)
-            return true;
-        report(
-            DiagnosticsEngine::Warning, "parentheses immediately inside single-arg call",
-            parenExpr->getLocStart())
-            << parenExpr->getSourceRange();
-    }
+    if (!parenExpr)
+        return true;
+    if (parenExpr->getLocStart().isMacroID())
+        return true;
+    // assignments need extra parentheses or they generate a compiler warning
+    auto binaryOp = dyn_cast<BinaryOperator>(parenExpr->getSubExpr());
+    if (binaryOp && binaryOp->getOpcode() == BO_Assign)
+        return true;
+    report(
+        DiagnosticsEngine::Warning, "parentheses immediately inside single-arg call",
+        parenExpr->getLocStart())
+        << parenExpr->getSourceRange();
+    return true;
+}
+
+bool UnnecessaryParen::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* callExpr)
+{
+    if (ignoreLocation(callExpr))
+        return true;
+    if (callExpr->getLocStart().isMacroID())
+        return true;
+    if (callExpr->getNumArgs() != 2)
+        return true;
+
+    // Same logic as CXXOperatorCallExpr::isAssignmentOp(), which our supported clang
+    // doesn't have yet.
+    auto Opc = callExpr->getOperator();
+    if (Opc != OO_Equal && Opc != OO_StarEqual &&
+        Opc != OO_SlashEqual && Opc != OO_PercentEqual &&
+        Opc != OO_PlusEqual && Opc != OO_MinusEqual &&
+        Opc != OO_LessLessEqual && Opc != OO_GreaterGreaterEqual &&
+        Opc != OO_AmpEqual && Opc != OO_CaretEqual &&
+        Opc != OO_PipeEqual)
+        return true;
+
+    auto parenExpr = dyn_cast<ParenExpr>(ignoreAllImplicit(callExpr->getArg(1)));
+    if (!parenExpr)
+        return true;
+    if (parenExpr->getLocStart().isMacroID())
+        return true;
+    // Sometimes parentheses make the RHS of an assignment easier to read by
+    // visually disambiguating the = from a call to ==
+    auto sub = parenExpr->getSubExpr();
+    if (isa<BinaryOperator>(sub)
+        || isa<CXXOperatorCallExpr>(sub)
+        || isa<ConditionalOperator>(sub))
+        return true;
+
+    report(
+        DiagnosticsEngine::Warning, "parentheses immediately inside assignment",
+        parenExpr->getLocStart())
+        << parenExpr->getSourceRange();
+    return true;
+}
+
+bool UnnecessaryParen::VisitVarDecl(const VarDecl* varDecl)
+{
+    if (ignoreLocation(varDecl))
+        return true;
+    if (varDecl->getLocStart().isMacroID())
+        return true;
+    if (!varDecl->getInit())
+        return true;
+
+    auto parenExpr = dyn_cast<ParenExpr>(ignoreAllImplicit(varDecl->getInit()));
+    if (!parenExpr)
+        return true;
+    if (parenExpr->getLocStart().isMacroID())
+        return true;
+    auto sub = parenExpr->getSubExpr();
+    if (isa<BinaryOperator>(sub)
+        || isa<CXXOperatorCallExpr>(sub)
+        || isa<ConditionalOperator>(sub)
+            // these two are for "parentheses were disambiguated as a function declaration [-Werror,-Wvexing-parse]"
+        || isa<CXXBindTemporaryExpr>(sub)
+        || isa<CXXFunctionalCastExpr>(sub))
+        return true;
+
+//varDecl->dump();
+
+    report(
+        DiagnosticsEngine::Warning, "parentheses immediately inside vardecl statement",
+        parenExpr->getLocStart())
+        << parenExpr->getSourceRange();
     return true;
 }