-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] add readability-redundant-parentheses #159911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] add readability-redundant-parentheses #159911
Conversation
This check wants to detect a common happened case that forgetting to remove parenthese during modifying code.
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesThis check wants to detect a common happened case that forgetting to Full diff: https://github.com/llvm/llvm-project/pull/159911.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 4b4c49d3b17d1..0d0641c4b22bf 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -44,6 +44,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
RedundantDeclarationCheck.cpp
RedundantFunctionPtrDereferenceCheck.cpp
RedundantMemberInitCheck.cpp
+ RedundantParenthesesCheck.cpp
RedundantPreprocessorCheck.cpp
RedundantSmartptrGetCheck.cpp
RedundantStringCStrCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d01882dfc9daa..fcfac05b000e4 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -47,6 +47,7 @@
#include "RedundantFunctionPtrDereferenceCheck.h"
#include "RedundantInlineSpecifierCheck.h"
#include "RedundantMemberInitCheck.h"
+#include "RedundantParenthesesCheck.h"
#include "RedundantPreprocessorCheck.h"
#include "RedundantSmartptrGetCheck.h"
#include "RedundantStringCStrCheck.h"
@@ -138,6 +139,8 @@ class ReadabilityModule : public ClangTidyModule {
"readability-redundant-function-ptr-dereference");
CheckFactories.registerCheck<RedundantMemberInitCheck>(
"readability-redundant-member-init");
+ CheckFactories.registerCheck<RedundantParenthesesCheck>(
+ "readability-redundant-parentheses");
CheckFactories.registerCheck<RedundantPreprocessorCheck>(
"readability-redundant-preprocessor");
CheckFactories.registerCheck<ReferenceToConstructedTemporaryCheck>(
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
new file mode 100644
index 0000000000000..6517f68216787
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
@@ -0,0 +1,54 @@
+
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantParenthesesCheck.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include <cassert>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+namespace {
+
+AST_MATCHER_P(ParenExpr, subExpr, ast_matchers::internal::Matcher<Expr>,
+ InnerMatcher) {
+ return InnerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+} // namespace
+
+void RedundantParenthesesCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ parenExpr(subExpr(anyOf(parenExpr(), integerLiteral(), floatLiteral(),
+ characterLiteral(), cxxBoolLiteral(),
+ stringLiteral(), declRefExpr())),
+ unless(
+ // sizeof(...) is common used.
+ hasParent(unaryExprOrTypeTraitExpr())))
+ .bind("dup"),
+ this);
+}
+
+void RedundantParenthesesCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *PE = Result.Nodes.getNodeAs<ParenExpr>("dup");
+ assert(PE);
+ const Expr *E = PE->getSubExpr();
+ if (PE->getLParen().isMacroID() || PE->getRParen().isMacroID() ||
+ E->getBeginLoc().isMacroID() || E->getEndLoc().isMacroID())
+ return;
+ diag(PE->getBeginLoc(), "redundant parentheses around expression")
+ << FixItHint::CreateRemoval(PE->getLParen())
+ << FixItHint::CreateRemoval(PE->getRParen());
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.h b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.h
new file mode 100644
index 0000000000000..8806fb391ba26
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.h
@@ -0,0 +1,31 @@
+
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPARENTHESESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPARENTHESESCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Detect redundant parentheses.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-parentheses.html
+class RedundantParenthesesCheck : public ClangTidyCheck {
+public:
+ RedundantParenthesesCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPARENTHESESCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a4652a7a54858..1dbacaf061b73 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -203,6 +203,11 @@ New checks
Finds virtual function overrides with different visibility than the function
in the base class.
+- New :doc:`readability-redundant-parentheses
+ <clang-tidy/checks/readability/redundant-parentheses>` check.
+
+ Detect redundant parentheses.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e06849c419389..51d968895044c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -404,6 +404,7 @@ Clang-Tidy Checks
:doc:`readability-redundant-function-ptr-dereference <readability/redundant-function-ptr-dereference>`, "Yes"
:doc:`readability-redundant-inline-specifier <readability/redundant-inline-specifier>`, "Yes"
:doc:`readability-redundant-member-init <readability/redundant-member-init>`, "Yes"
+ :doc:`readability-redundant-parentheses <readability/redundant-parentheses>`, "Yes"
:doc:`readability-redundant-preprocessor <readability/redundant-preprocessor>`,
:doc:`readability-redundant-smartptr-get <readability/redundant-smartptr-get>`, "Yes"
:doc:`readability-redundant-string-cstr <readability/redundant-string-cstr>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst
new file mode 100644
index 0000000000000..09f9113b89cbd
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-parentheses
+
+readability-redundant-parentheses
+=================================
+
+Detect redundant parentheses.
+
+When modifying the code, one often forgets to remove the corresponding parentheses.
+This results in overly lengthy code. When the expression is complex, finding
+the matching parentheses becomes particularly difficult.
+
+Example
+-------
+
+.. code-block:: c++
+
+ (1);
+ ((a + 2)) * 3;
+ (a);
+ ("aaa");
+
+Currently this check does not take into account the precedence of operations.
+Even if the expression within the parentheses has a higher priority than that
+outside the parentheses, in other words, removing the parentheses will not
+affect the semantic.
+
+.. code-block:: c++
+
+ int a = (1 * 2) + 3; // no warning
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp
new file mode 100644
index 0000000000000..017f22ec75721
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s readability-redundant-parentheses %t
+
+void parenExpr() {
+ 1 + 1;
+ (1 + 1);
+ ((1 + 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: (1 + 1);
+ (((1 + 1)));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-MESSAGES: :[[@LINE-2]]:4: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: (1 + 1);
+ ((((1 + 1))));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-MESSAGES: :[[@LINE-2]]:4: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: (1 + 1);
+}
+
+#define EXP (1 + 1)
+#define PAREN(e) (e)
+void parenExprWithMacro() {
+ EXP; // 1
+ (EXP); // 2
+ ((EXP)); // 3
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: (EXP); // 3
+ PAREN((1));
+}
+
+void constant() {
+ (1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: 1;
+ (1.0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: 1.0;
+ (true);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: true;
+ (',');
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: ',';
+ ("v4");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: "v4";
+}
+
+void declRefExpr(int a) {
+ (a);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant parentheses around expression [readability-redundant-parentheses]
+ // CHECK-FIXES: a;
+}
+
+void exceptions() {
+ sizeof(1);
+ alignof(2);
+}
|
clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.h
Outdated
Show resolved
Hide resolved
Clang-format has an option like this already. Unlike clang-format, we have full semantic information, so this check has the potential to be more reliable, but I’m concerned about duplicating functionality. What do you think? |
I don't see a problem in duplicating functionality given Generally speaking, if |
clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp
Show resolved
Hide resolved
RedundantParenthesesCheck(StringRef Name, ClangTidyContext *Context) | ||
: ClangTidyCheck(Name, Context) {} | ||
void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
void check(const ast_matchers::MatchFinder::MatchResult &Result) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we set isLanguageVersionSupported
to CPlusPlus || C99
otherwise the check may run on ObjC, which may have different semantics I guess..
I wonder, could we make the default to CPlusPlus || C99
instead of true
here:
llvm-project/clang-tools-extra/clang-tidy/ClangTidyCheck.h
Lines 68 to 70 in c12f08f
virtual bool isLanguageVersionSupported(const LangOptions &LangOpts) const { | |
return true; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, could we make the default to CPlusPlus || C99 instead of true here
It needs RFC IMO. I have never written OC so I don't know the ecosystem of OC. Is clang-tidy a common used tools in OC's world?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, could we make the default to CPlusPlus || C99 instead of true here
I have never written OC so I don't know the ecosystem of OC. Is clang-tidy a common used tools in OC's world?
I've never used OC too, so we'd definitely need an RFC for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is to disable the check for OC, shouldn't we just do return !LangOpts.ObjC
? It expresses intent more clearly. (Although if ObjC
isn't set for Objective-C++, that won't work.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have further discussion in https://discourse.llvm.org/t/rfc-clang-tidy-which-language-should-be-supported-by-default/88424
That sounds reasonable, thank you |
clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.h
Outdated
Show resolved
Hide resolved
…heck.h Co-authored-by: Victor Chernyakin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with couple nits
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/RedundantParenthesesCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst
Outdated
Show resolved
Hide resolved
RedundantParenthesesCheck(StringRef Name, ClangTidyContext *Context) | ||
: ClangTidyCheck(Name, Context) {} | ||
void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
void check(const ast_matchers::MatchFinder::MatchResult &Result) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is to disable the check for OC, shouldn't we just do return !LangOpts.ObjC
? It expresses intent more clearly. (Although if ObjC
isn't set for Objective-C++, that won't work.)
…-parentheses.rst Co-authored-by: Baranov Victor <[email protected]>
…-parentheses.rst Co-authored-by: Baranov Victor <[email protected]>
…-parentheses.rst Co-authored-by: Victor Chernyakin <[email protected]>
…vm-project into RedundantParenthesesCheck
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/18428 Here is the relevant piece of the build log for the reference
|
This check wants to detect a common happened case that forgetting to remove parenthese during modifying code. --------- Co-authored-by: Victor Chernyakin <[email protected]> Co-authored-by: Baranov Victor <[email protected]>
This check wants to detect a common happened case that forgetting to
remove parenthese during modifying code.