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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 13 01:18:46 PDT 2019


hokein added a comment.

looks most good to me, a few nits.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6488
+      }
+      return InnerMatcher.matches(*CtorExpr, Finder, Builder);
+    }
----------------
This `return` statement is not needed.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:16
 #include "gtest/gtest.h"
+#include <unordered_map>
 
----------------
looks like the `include` is not used?


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:153
+  case LanguageMode::Cxx2aOrLater:
+    LangModes = {LanguageMode::Cxx2a};
+  }
----------------
nit: add a llvm_unreachable for `default`.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:177
+        matchesConditionally(Code, AMatcher, ExpectMatch, LangModeArg);
+    if (!Result) {
+      return Result;
----------------
nit: no `{}` needed for a single-statement body.


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