teach unnecessaryparen loplugin about identifiers

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

2 files changed, 67 insertions, 0 deletions

diff --git a/compilerplugins/clang/test/unnecessaryparen.cxx b/compilerplugins/clang/test/unnecessaryparen.cxx
index 77cb6bb87168..85be13848b22 100644
--- a/compilerplugins/clang/test/unnecessaryparen.cxx
+++ b/compilerplugins/clang/test/unnecessaryparen.cxx
@@ -9,6 +9,8 @@
 
 bool foo(int);
 
+enum class EFoo { Bar };
+
 int main()
 {
     int x = 1;
@@ -17,6 +19,19 @@ int main()
     if ((foo(1))) foo(2); // expected-error {{parentheses immediately inside if statement [loplugin:unnecessaryparen]}}
 
     foo((1)); // expected-error {{parentheses immediately inside single-arg call [loplugin:unnecessaryparen]}}
+
+    int y = (x); // expected-error {{unnecessary parentheses around identifier [loplugin:unnecessaryparen]}}
+    (void)y;
+
+    // lots of our code uses this style, which I'm loathe to bulk-fix as yet
+    EFoo foo = EFoo::Bar;
+    switch (foo) {
+        case (EFoo::Bar): break;
+    }
+
+    // lots of our code uses this style, which I'm loathe to bulk-fix as yet
+    int z = (y) ? 1 : 0;
+    (void)z;
 };
 
 /* 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 55dac9523870..40faa88b3157 100644
--- a/compilerplugins/clang/unnecessaryparen.cxx
+++ b/compilerplugins/clang/unnecessaryparen.cxx
@@ -56,16 +56,50 @@ public:
     bool VisitWhileStmt(const WhileStmt *);
     bool VisitSwitchStmt(const SwitchStmt *);
     bool VisitCallExpr(const CallExpr *);
+    bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *);
+    bool TraverseCaseStmt(CaseStmt *);
+    bool TraverseConditionalOperator(ConditionalOperator *);
 private:
     void VisitSomeStmt(const Stmt *parent, const Expr* cond, StringRef stmtName);
+    Expr* insideCaseStmt = nullptr;
+    Expr* insideConditionalOperator = nullptr;
 };
 
+bool UnnecessaryParen::TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *)
+{
+    // for some reason, the parentheses in an expression like "sizeof(x)" actually show up
+    // in the AST, so just ignore that part of the AST
+    return true;
+}
+
+bool UnnecessaryParen::TraverseCaseStmt(CaseStmt * caseStmt)
+{
+    auto old = insideCaseStmt;
+    insideCaseStmt = caseStmt->getLHS()->IgnoreImpCasts();
+    bool ret = RecursiveASTVisitor::TraverseCaseStmt(caseStmt);
+    insideCaseStmt = old;
+    return ret;
+}
+
+bool UnnecessaryParen::TraverseConditionalOperator(ConditionalOperator * conditionalOperator)
+{
+    auto old = insideConditionalOperator;
+    insideConditionalOperator = conditionalOperator->getCond()->IgnoreImpCasts();
+    bool ret = RecursiveASTVisitor::TraverseConditionalOperator(conditionalOperator);
+    insideConditionalOperator = old;
+    return ret;
+}
+
 bool UnnecessaryParen::VisitParenExpr(const ParenExpr* parenExpr)
 {
     if (ignoreLocation(parenExpr))
         return true;
     if (parenExpr->getLocStart().isMacroID())
         return true;
+    if (insideCaseStmt && parenExpr == insideCaseStmt)
+        return true;
+    if (insideConditionalOperator && parenExpr == insideConditionalOperator)
+        return true;
 
     auto subParenExpr = dyn_cast<ParenExpr>(parenExpr->getSubExpr()->IgnoreImpCasts());
     if (subParenExpr) {
@@ -76,6 +110,24 @@ bool UnnecessaryParen::VisitParenExpr(const ParenExpr* parenExpr)
             parenExpr->getLocStart())
             << parenExpr->getSourceRange();
     }
+
+    auto declRefExpr = dyn_cast<DeclRefExpr>(parenExpr->getSubExpr()->IgnoreImpCasts());
+    if (declRefExpr) {
+        if (declRefExpr->getLocStart().isMacroID())
+            return true;
+
+        // hack for BAD_CAST macro
+        SourceManager& SM = compiler.getSourceManager();
+        const char *p1 = SM.getCharacterData( declRefExpr->getLocStart().getLocWithOffset(-10) );
+        const char *p2 = SM.getCharacterData( declRefExpr->getLocStart() );
+        if ( std::string(p1, p2 - p1).find("BAD_CAST") != std::string::npos )
+            return true;
+
+        report(
+            DiagnosticsEngine::Warning, "unnecessary parentheses around identifier",
+            parenExpr->getLocStart())
+            << parenExpr->getSourceRange();
+    }
     return true;
 }