[PATCH] D113943: Add `withIntrospection` matcher.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D113943#3184076 , @avogelsgesang 
wrote:

> Nit:
> Personally, I would prefer the name `withDebugOutput` over 
> `withIntrospection`, as it more clearly describes the intent of this matcher.
> At least to me, "introspection" is usually something done programatically, 
> i.e. the matcher somehow reflects about its own state, but in this case all 
> we are doing is printing some debug output to stderr.

FWIW, I tend to agree that "introspection" is a bit of an odd choice for naming 
here. `withDebugOutput` would be an improvement, but `withPrefixAndSuffix` or 
something along those lines would also work.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3000-3001
+  const internal::Matcher ) {
+  return internal::Matcher(new internal::WithIntrospectionMatcher(
+  BeforeTag, AfterTag, InnerMatcher));
+}

avogelsgesang wrote:
> std::move(BeforeTag), std::move(AfterTag)
> 
> No need to create additional copies of those strings. Here and other places
Alternatively, it'd be great if we can switch these to use `StringRef` to avoid 
copies.



Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1064-1066
+  explicit WithIntrospectionMatcher(std::string _BeforeTag,
+std::string _AfterTag,
+internal::Matcher _InnerMatcher)

All of those identifiers introduce UB because they're reserved, dropping the 
underscore solves the issue (and, oddly, matches our terrible coding style 
where we let ctor parameters shadow the names of member variables).



Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1784
+  internal::PolymorphicMatcher
+  _InnerMatcher)
+  : InnerMatcher(_InnerMatcher) {}




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113943: Add `withIntrospection` matcher.

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Nit:
Personally, I would prefer the name `withDebugOutput` over `withIntrospection`, 
as it more clearly describes the intent of this matcher.
At least to me, "introspection" is usually something done programatically, i.e. 
the matcher somehow reflects about its own state, but in this case all we are 
doing is printing some debug output to stderr.

Code looks good to me, but I don't know the LLVM coding guidelines and hence 
can't provide an actual code review




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3000-3001
+  const internal::Matcher ) {
+  return internal::Matcher(new internal::WithIntrospectionMatcher(
+  BeforeTag, AfterTag, InnerMatcher));
+}

std::move(BeforeTag), std::move(AfterTag)

No need to create additional copies of those strings. Here and other places


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits