[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