[PATCH] D63149: Added AST matcher for ignoring elidable constructors

Dmitri Gribenko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 12 07:00:49 PDT 2019


gribozavr added inline comments.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:600
+  EXPECT_TRUE(matches(code2, matcher2, LanguageMode::CXX11OrLater));
+  EXPECT_TRUE(matches(code3, matcher3, LanguageMode::CXX11OrLater));
+}
----------------
Please inline the variables that are used once.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:61
+enum class LanguageMode : int {
+  CXX11 = 0,
+  CXX14 = 1,
----------------
Please drop the explicit values.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:64
+  CXX17 = 2,
+  CXX20 = 3,
+  CXX11OrLater,
----------------
2a


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:68
+  CXX17OrLater,
+  CXX20OrLater
+};
----------------
"2a" (we don't know the year yet)


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:144
+  case LanguageMode::CXX11OrLater:
+    LangModes = {LanguageMode::CXX11, LanguageMode::CXX14, LanguageMode::CXX17, LanguageMode::CXX20};
+    break;
----------------
80 columns.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:154
+    LangModes = {LanguageMode::CXX20};
+  };
+
----------------
No need for semicolon.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:157
+  for (auto Mode : LangModes) {
+    auto LangModeArg = LangModeStrings[static_cast<int>(Mode)];
+    auto Result =
----------------
I'd say it is too clever, and suggest to write an explicit switch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63149/new/

https://reviews.llvm.org/D63149





More information about the cfe-commits mailing list