clang-tidy: strip useless parens from `return` statements

[oleg.smolsky]

Summary

• this adds a new check: readability-return-with-redundant-parens

We can find these with the AST matcher. We exclude macros.

[oleg.smolsky]

Drop the trailing \n from the main C++ file

[Eugene.Zelenko]

Please add documentation and mention new check in Release Notes.

[njames93]

The idea of this check is good, but restricting it to only return statements seems baffling. A general check that could remove useless parens would have a lot more value.

[oleg.smolsky]

Add docs.

[Eugene.Zelenko]

Entry in Release Notes is still missing.

clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst

5

Please make same length as check name.

7

Please use double back-ticks for language constructs.

[oleg.smolsky]

Add a release note.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

110

Please use double back-ticks for language constructs.

[oleg.smolsky]

In D131985#3727436, @njames93 wrote:

The idea of this check is good, but restricting it to only return statements seems baffling. A general check that could remove useless parens would have a lot more value.

Of course a more general check would be more generally useful. Yet that requires a lot more code as handling many more contexts is involved in C++.

Basically this change addresses a concrete, somewhat wide-spread silly habit that exists in the wild.

[oleg.smolsky]

Improve mark up in the docs.

[oleg.smolsky]

Thanks for the quick review, Eugene!

clang-tools-extra/docs/ReleaseNotes.rst

110

Done.

clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst

5

Done

7

Done.

[Eugene.Zelenko]

In D131985#3727544, @oleg.smolsky wrote:

In D131985#3727436, @njames93 wrote:

The idea of this check is good, but restricting it to only return statements seems baffling. A general check that could remove useless parens would have a lot more value.

Of course a more general check would be more generally useful. Yet that requires a lot more code as handling many more contexts in involved in C++.

Basically this change addresses a concrete, somewhat wide-spread silly habit that exists in the wild.

I recently observed in the wild patterns like:

std::vector<std::string> data;

for (std::vector<std::string>::iterator it = data.begin(); it != data.end(); ++it)
  std::cout << (*it) << std::endl;

[oleg.smolsky]

In D131985#3727568, @Eugene.Zelenko wrote:

In D131985#3727544, @oleg.smolsky wrote:

In D131985#3727436, @njames93 wrote:

The idea of this check is good, but restricting it to only return statements seems baffling. A general check that could remove useless parens would have a lot more value.

Of course a more general check would be more generally useful. Yet that requires a lot more code as handling many more contexts in involved in C++.

Basically this change addresses a concrete, somewhat wide-spread silly habit that exists in the wild.

I recently observed in the wild patterns like:

std::vector<std::string> data;

for (std::vector<std::string>::iterator it = data.begin(); it != data.end(); ++it)
  std::cout << (*it) << std::endl;

While I have not seen that one, I've seen something similar, with even more involved syntax:

for (auto it = data.begin(); it != data.end(); ++it)
   std::cout << (*it).something << std::endl;

people write wild syntax...

[oleg.smolsky]

Add AST matchers for the if statements and improve the actual check implementation.

[oleg.smolsky]

Simplify and generalize the AST matcher. We just want to find all return statements in function definitions.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantPreprocessorCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantStringInitCheck.cpp
+  ReturnWithRedundantParensCheck.cpp
   SimplifyBooleanExprCheck.cpp
   SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   SuspiciousCallArgumentCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
   UppercaseLiteralSuffixCheck.cpp

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantPreprocessorCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
+#include "ReturnWithRedundantParensCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousCallArgumentCheck>(
         "readability-suspicious-call-argument");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
         "readability-uppercase-literal-suffix");
     CheckFactories.registerCheck<UseAnyOfAllOfCheck>(
         "readability-use-anyofallof");
+    CheckFactories.registerCheck<ReturnWithRedundantParensCheck>(
+        "readability-return-with-redundant-parens");
   }
 };
 
 // Register the ReadabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
 } // namespace readability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h

+//===--- ReturnWithRedundantParensCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H

clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp

+//===--- ReturnWithRedundantParensCheck.cpp - clang-tidy --------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReturnWithRedundantParensCheck.h"
+#include "clang/Lex/Lexer.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+bool isLocationInMacroExpansion(const SourceManager &SM, SourceLocation Loc) {
+  return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
+}
+} // namespace
+
+void ReturnWithRedundantParensCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      returnStmt(has(expr(hasAncestor(functionDecl(
+                     allOf(isDefinition(), returns(unless(voidType()))))))))
+          .bind("return"),
+      this);
+}
+
+void ReturnWithRedundantParensCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Return = Result.Nodes.getNodeAs<ReturnStmt>("return");
+  assert(Return);
+  SourceManager &SM = *Result.SourceManager;
+
+  if (isLocationInMacroExpansion(SM, Return->getSourceRange().getBegin()))
+    return;
+
+  assert(!Return->children().empty());
+  auto ChildExpr = *Return->children().begin();
+  if (!isa<ParenExpr>(ChildExpr))
+    return;
+
+  auto Expr = cast<ParenExpr>(ChildExpr);
+  auto LParen = Expr->getLParen();
+  assert(LParen.isValid());
+  auto RParen = Expr->getRParen();
+  assert(RParen.isValid());
+  auto RemovedRange1 =
+      CharSourceRange::getCharRange(LParen, LParen.getLocWithOffset(1));
+  auto RemovedRange2 =
+      CharSourceRange::getCharRange(RParen, RParen.getLocWithOffset(1));
+
+  diag(LParen, "Redundant parens in the 'return' statement")
+      << FixItHint::CreateRemoval(RemovedRange1)
+      << FixItHint::CreateRemoval(RemovedRange2);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 New checks
 ^^^^^^^^^^
 
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-return-with-redundant-parens
+  <clang-tidy/checks/readability/return-with-redundant-parens>` check.
+
+  Checks for ``return`` statements with redundant parenthesis.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `readability-redundant-control-flow <readability/redundant-control-flow.html>`_, "Yes"
    `readability-redundant-declaration <readability/redundant-declaration.html>`_, "Yes"
    `readability-redundant-function-ptr-dereference <readability/redundant-function-ptr-dereference.html>`_, "Yes"
    `readability-redundant-member-init <readability/redundant-member-init.html>`_, "Yes"
    `readability-redundant-preprocessor <readability/redundant-preprocessor.html>`_,
    `readability-redundant-smartptr-get <readability/redundant-smartptr-get.html>`_, "Yes"
    `readability-redundant-string-cstr <readability/redundant-string-cstr.html>`_, "Yes"
    `readability-redundant-string-init <readability/redundant-string-init.html>`_, "Yes"
+   `readability-return-with-redundant-parens <readability/return-with-redundant-parens.html>`_, "Yes"
    `readability-simplify-boolean-expr <readability/simplify-boolean-expr.html>`_, "Yes"
    `readability-simplify-subscript-expr <readability/simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability/static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability/static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability/string-compare.html>`_, "Yes"
    `readability-suspicious-call-argument <readability/suspicious-call-argument.html>`_,
    `readability-uniqueptr-delete-release <readability/uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability/uppercase-literal-suffix.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst

+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+========================================
+
+Checks for ``return`` statements with redundant parenthesis.
+
+Example
+-------
+
+.. code-block:: c++
+
+  void f() {
+    return (1);  // Note, the parenthesis here are redundant.
+  }
+

clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+bool helper();
+
+bool if1(int *p) {
+  if (p) return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) return helper();{{$}}
+}
+
+bool if2(int *p) {
+  if (p) p = nullptr; else return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) p = nullptr; else return helper();{{$}}
+}
+
+bool if3(int *arg) {
+  if (arg) {
+    return(helper());
+    // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+    // CHECK-FIXES: {{^ +}}return helper();{{$}}
+  } else {
+    return(helper());
+    // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+    // CHECK-FIXES: {{^ +}}return helper();{{$}}
+  }
+
+  if (arg) {
+    if (helper()) {
+      return(helper());
+      // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+      // CHECK-FIXES: {{^ +}}return helper();{{$}}
+    }
+  }
+}