[clang-tidy] Improve misc-redundant-expression and decrease false-positive

[etienneb]

Summary

This patch is adding support for conditional expression and overloaded operators.

To decrease false-positive, this patch is adding a list of banned macro names that
has multiple variant with same integer value.

Also fixed support for template instantiation and added an unittest.

[etienneb]

nit

[etienneb]

adding unittests

[alexfh]

clang-tidy/misc/RedundantExpressionCheck.cpp

27 ↗

(On Diff #55526)

Now that you have pulled an API for this, I'll wait until you update this patch.

[etienneb]

use new API

[etienneb]

fix bad upload

[alexfh]

LG.

clang-tidy/misc/RedundantExpressionCheck.cpp

140 ↗

(On Diff #56912)

Is there a way to not create a set on each call to the matcher? There also might be a more suitable container in llvm/ADT.

189 ↗

(On Diff #56912)

We should add a hasAnyOverloadedOperatorName matcher at some point.

[etienneb]

We should add a hasAnyOverloadedOperatorName matcher at some point.

I proposed it to sbenza@ and he told me that the reason he did HasAnyName was for speed issue.
I can't tell if that one has speed issue too.

But, otherwise, I agree... for code readability... we should do it.

[etienneb]

remove useless set creation

[etienneb]

fix naming nits

[etienneb]

+ Removed copy of a vector to a set
+ Fixed some minor coding-style issues

clang-tidy/misc/RedundantExpressionCheck.cpp

140 ↗

(On Diff #56912)

Some ideas:

1. We may assume the vector is small and do the lookup.
2. We may assume a sorted vector and do a binary search.
3. The matcher could receive a set instead of a vector and lifting out the copy.

Implemented 3)

[alexfh]

clang-tidy/misc/RedundantExpressionCheck.cpp

139 ↗

(On Diff #56937)

Ah, found it: llvm/ADT/StringSet.h seems to be a better choice here.

include/clang/Tooling/FixIt.h

+//===--- FixIt.h - FixIt Hint utilities -------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements functions to ease source rewriting from AST-nodes.
+//
+//  Example swapping A and B expressions:
+//
+//    Expr *A, *B;
+//    tooling::fixit::createReplacement(*A, *B);
+//    tooling::fixit::createReplacement(*B, *A);
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_FIXIT_H
+#define LLVM_CLANG_TOOLING_FIXIT_H
+
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context);
+
+/// \brief Returns the SourceRange of a SourceRange. This identity function is
+///        used by the following template abstractions.
+inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+
+/// \brief Returns the SourceRange of the token at Location \p Loc.
+inline SourceRange getSourceRange(const SourceLocation &Loc) {
+  return SourceRange(Loc);
+}
+
+/// \brief Returns the SourceRange of an given Node. \p Node is typically a
+///        'Stmt', 'Expr' or a 'Decl'.
+template <typename T> SourceRange getSourceRange(const T &Node) {
+  return Node.getSourceRange();
+}
+} // end namespace internal
+
+// \brief Returns a textual representation of \p Node.
+template <typename T>
+StringRef getText(const T &Node, const ASTContext &Context) {
+  return internal::getText(internal::getSourceRange(Node), Context);
+}
+
+// \brief Returns a FixItHint to remove \p Node.
+// TODO: Add support for related syntactical elements (i.e. comments, ...).
+template <typename T> FixItHint createRemoval(const T &Node) {
+  return FixItHint::CreateRemoval(internal::getSourceRange(Node));
+}
+
+// \brief Returns a FixItHint to replace \p Destination by \p Source.
+template <typename D, typename S>
+FixItHint createReplacement(const D &Destination, const S &Source,
+                            const ASTContext &Context) {
+  return FixItHint::CreateReplacement(internal::getSourceRange(Destination),
+                                      getText(Source, Context));
+}
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_FIXINT_H

lib/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_subdirectory(Core)
 
 add_clang_library(clangTooling
   ArgumentsAdjusters.cpp
   CommonOptionsParser.cpp
   CompilationDatabase.cpp
   FileMatchTrie.cpp
+  FixIt.cpp
   JSONCompilationDatabase.cpp
   Refactoring.cpp
   RefactoringCallbacks.cpp
   Tooling.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangDriver
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangToolingCore
   )

lib/Tooling/FixIt.cpp

+//===--- FixIt.cpp - FixIt Hint utilities -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains implementations of utitilies to ease source code rewriting
+// by providing helper functions related to FixItHint.
+//
+//===----------------------------------------------------------------------===//
+#include "clang/Tooling/FixIt.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                              Context.getSourceManager(),
+                              Context.getLangOpts());
+}
+} // end namespace internal
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang

unittests/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   Support
   )
 
 # By default MSVC has a 2^16 limit on the number of sections in an object file,
 # and this needs more than that.
 if (MSVC)
   set_source_files_properties(RecursiveASTVisitorTestExprVisitor.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  FixItTest.cpp  
   LookupTest.cpp
-  ToolingTest.cpp
+  QualTypeNamesTest.cpp
   RecursiveASTVisitorTest.cpp
   RecursiveASTVisitorTestCallVisitor.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestExprVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
-  RefactoringTest.cpp
-  RewriterTest.cpp
   RefactoringCallbacksTest.cpp
+  RefactoringTest.cpp
   ReplacementsYamlTest.cpp
-  QualTypeNamesTest.cpp
+  RewriterTest.cpp
+  ToolingTest.cpp
   )
 
 target_link_libraries(ToolingTests
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangTooling
   clangToolingCore
   )

unittests/Tooling/FixItTest.cpp

+//===- unittest/Tooling/FixitTest.cpp ------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestVisitor.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang;
+
+using tooling::fixit::getText;
+using tooling::fixit::createRemoval;
+using tooling::fixit::createReplacement;
+
+namespace {
+
+struct CallsVisitor : TestVisitor<CallsVisitor> {
+  bool VisitCallExpr(CallExpr *Expr) {
+    OnCall(Expr, Context);
+    return true;
+  }
+
+  std::function<void(CallExpr *, ASTContext *Context)> OnCall;
+};
+
+std::string LocationToString(SourceLocation Loc, ASTContext *Context) {
+  return Loc.printToString(Context->getSourceManager());
+}
+
+TEST(FixItTest, getText) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo(x, y)", getText(*CE, *Context));
+    EXPECT_EQ("foo(x, y)", getText(CE->getSourceRange(), *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getText(*P0, *Context));
+    EXPECT_EQ("y", getText(*P1, *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("APPLY(foo, x, y)", getText(*CE, *Context));
+  };
+  Visitor.runOver("#define APPLY(f, x, y) f(x, y)\n"
+                  "void foo(int x, int y) { APPLY(foo, x, y); }");
+}
+
+TEST(FixItTest, getTextWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("F OO", getText(*CE, *Context));
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("", getText(*P0, *Context));
+    EXPECT_EQ("", getText(*P1, *Context));
+  };
+  Visitor.runOver("#define F foo(\n"
+                  "#define OO x, y)\n"
+                  "void foo(int x, int y) { F OO ; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("", getText(*CE, *Context));
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getText(*P0, *Context));
+    EXPECT_EQ("y", getText(*P1, *Context));
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+}
+
+TEST(FixItTest, createRemoval) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("foo(x, y)", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = createRemoval(*P0);
+    EXPECT_EQ("x", getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint0.CodeToInsert.empty());
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = createRemoval(*P1);
+    EXPECT_EQ("y", getText(Hint1.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint1.CodeToInsert.empty());
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = createRemoval(*P0);
+    EXPECT_EQ("x + y", getText(Hint0.RemoveRange.getAsRange(), *Context));
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = createRemoval(*P1);
+    EXPECT_EQ("y + x", getText(Hint1.RemoveRange.getAsRange(), *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x + y, y + x); }");
+}
+
+TEST(FixItTest, createRemovalWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("FOO", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = createRemoval(*P0);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint0.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint0.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint0.CodeToInsert.empty());
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = createRemoval(*P1);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:20>",
+              LocationToString(Hint1.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:20>",
+              LocationToString(Hint1.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint1.CodeToInsert.empty());
+  };
+  Visitor.runOver("#define FOO foo(1, 1)\n"
+                  "void foo(int x, int y) { FOO; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:37>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:45>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+}
+
+TEST(FixItTest, createReplacement) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint0 = createReplacement(*P0, *P1, *Context);
+    FixItHint Hint1 = createReplacement(*P1, *P0, *Context);
+
+    // Validate Hint0 fields.
+    EXPECT_EQ("x", getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_EQ(Hint0.CodeToInsert, "y");
+
+    // Validate Hint1 fields.
+    EXPECT_EQ("y", getText(Hint1.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_EQ(Hint1.CodeToInsert, "x");
+  };
+
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+
+  Visitor.runOver("#define APPLY(f, x, y) f(x, y)\n"
+                  "void foo(int x, int y) { APPLY(foo, x, y); }");
+
+  Visitor.runOver("#define APPLY(f, P) f(P)\n"
+                  "#define PAIR(x, y) x, y\n"
+                  "void foo(int x, int y) { APPLY(foo, PAIR(x, y)); }\n");
+}
+
+TEST(FixItTest, createReplacementWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+  };
+
+  Visitor.runOver("#define FOO foo(1, 1)\n"
+                  "void foo(int x, int y) { FOO; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:2:30>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:2:30>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_EQ("y", Hint.CodeToInsert);
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("x + y", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_EQ("y + x", Hint.CodeToInsert);
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x + y, y + x); }");
+}
+
+} // end anonymous namespace