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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6455
 
+/// Matches expressions that match InnerMatcher after any elidable constructor are stripped off.
+///
----------------
80 columns


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6455
 
+/// Matches expressions that match InnerMatcher after any elidable constructor are stripped off.
+///
----------------
gribozavr wrote:
> 80 columns
It would help if you described why a user would want to skip the elidable constructor using this matcher rather than hardcoding the presence of such an AST node.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6457
+///
+/// Example matches the entire D1 = ... (matcher = cxxOperatorCallExpr(hasArgument(1, callExpr(hasArgument(0, ignoringElidableMoveConstructorCall(ignoringParenImpCasts(callExpr())))))))
+/// \code
----------------
I'd suggest to rephrase the example using the "Given:" format (see other comments around), and wrap the code to 80 columns.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:575
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+    "struct H {};"
----------------
Can you run these tests in both C++14 and C++17 modes?


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:628
+  EXPECT_TRUE(matches(
+    "void f(){"
+    "int D = 10;"
----------------
Need a space before the open brace, and indentation in the function body (in every test).


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:631
+    "}"
+  , matcher));
+}
----------------
Please clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63149