[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356675: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause 
handling (authored by lebedevri, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57112?vs=191697=191706#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57112

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/AST/ASTTypeTraits.h
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/AST/ASTTypeTraits.cpp
  cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: cfe/trunk/docs/LibASTMatchersReference.html
===
--- cfe/trunk/docs/LibASTMatchersReference.html
+++ cfe/trunk/docs/LibASTMatchersReference.html
@@ -645,6 +645,19 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPClause.html;>OMPClauseompDefaultClauseMatcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPDefaultClause.html;>OMPDefaultClause...
+Matches OpenMP ``default`` clause.
+
+Given
+
+  #pragma omp parallel default(none)
+  #pragma omp parallel default(shared)
+  #pragma omp parallel
+
+``ompDefaultClause()`` matches ``default(none)`` and ``default(shared)``.
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualTypequalTypeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1QualType.html;>QualType...
 Matches QualTypes in the clang AST.
 
@@ -3439,6 +3452,51 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPDefaultClause.html;>OMPDefaultClauseisNoneKind
+Matches if the OpenMP ``default`` clause has ``none`` kind specified.
+
+Given
+
+  #pragma omp parallel
+  #pragma omp parallel default(none)
+  #pragma omp parallel default(shared)
+
+``ompDefaultClause(isNoneKind())`` matches only ``default(none)``.
+
+
+
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPDefaultClause.html;>OMPDefaultClauseisSharedKind
+Matches if the OpenMP ``default`` clause has ``shared`` kind specified.
+
+Given
+
+  #pragma omp parallel
+  #pragma omp parallel default(none)
+  #pragma omp parallel default(shared)
+
+``ompDefaultClause(isSharedKind())`` matches only ``default(shared)``.
+
+
+
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPExecutableDirective.html;>OMPExecutableDirectiveisAllowedToContainClauseKindOpenMPClauseKind CKind
+Matches if the OpenMP directive is allowed to contain the specified OpenMP
+clause kind.
+
+Given
+
+  #pragma omp parallel
+  #pragma omp parallel for
+  #pragma omp  for
+
+`ompExecutableDirective(isAllowedToContainClause(OMPC_default))`` matches
+``omp parallel`` and ``omp parallel for``.
+
+If the matcher is use from clang-query, ``OpenMPClauseKind`` parameter
+should be passed as a quoted string. e.g.,
+``isAllowedToContainClauseKind("OMPC_default").``
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMessageExpr.html;>ObjCMessageExprargumentCountIsunsigned N
 Checks that a call expression or a constructor call expression has
 a specific number of arguments (including absent default arguments).
@@ -6163,6 +6221,19 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPExecutableDirective.html;>OMPExecutableDirectivehasAnyClauseMatcherhttps://clang.llvm.org/doxygen/classclang_1_1OMPClause.html;>OMPClause InnerMatcher
+Matches any clause in an OpenMP directive.
+
+Given
+
+  #pragma omp parallel
+  #pragma omp parallel default(none)
+
+``ompExecutableDirective(hasAnyClause(anything()))`` matches
+``omp parallel default(none)``.
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMessageExpr.html;>ObjCMessageExprhasAnyArgumentMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr InnerMatcher
 Matches any argument of a call expression or a constructor call
 expression, or an ObjC-message-send expression.
Index: cfe/trunk/include/clang/AST/ASTTypeTraits.h
===
--- cfe/trunk/include/clang/AST/ASTTypeTraits.h
+++ cfe/trunk/include/clang/AST/ASTTypeTraits.h
@@ -18,6 +18,7 @@
 #include "clang/AST/ASTFwd.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/OpenMPClause.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TypeLoc.h"
@@ -58,6 +59,7 @@
   static ASTNodeKind getFromNode(const Decl );
   static ASTNodeKind getFromNode(const Stmt );
   static ASTNodeKind getFromNode(const Type );
+  static ASTNodeKind getFromNode(const OMPClause );
   /// \}
 
   /// Returns \c true if \c this and \c Other represent the same kind.
@@ -136,6 +138,9 @@

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6402
+///
+/// ``ompExecutableDirective(hasClause(anything()))`` matches
+/// ``omp parallel default(none)``.

aaron.ballman wrote:
> hasAnyClause()
whoops, thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57112#1437954 , @aaron.ballman 
wrote:

> LGTM


Great, thank you for the review!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 191697.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Comment nit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112

Files:
  docs/LibASTMatchersReference.html
  include/clang/AST/ASTTypeTraits.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTTypeTraits.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1789,5 +1789,43 @@
   EXPECT_TRUE(notMatchesWithOpenMP(Source2, Matcher));
 }
 
+TEST(OMPDefaultClause, Matches) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(ompDefaultClause()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2274,5 +2274,173 @@
 notMatches("int main2() {}", functionDecl(isMain(;
 }
 
+TEST(OMPExecutableDirective, hasClause) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(anything()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isNoneKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isNoneKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isSharedKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isSharedKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPExecutableDirective, isAllowedToContainClauseKind) {
+  auto Matcher =
+  ompExecutableDirective(isAllowedToContainClauseKind(OMPC_default));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from a small docs nit (be sure to regenerate the docs after you fix 
it).




Comment at: include/clang/ASTMatchers/ASTMatchers.h:6402
+///
+/// ``ompExecutableDirective(hasClause(anything()))`` matches
+/// ``omp parallel default(none)``.

hasAnyClause()


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 191685.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

Address @aaron.ballman comment nit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112

Files:
  docs/LibASTMatchersReference.html
  include/clang/AST/ASTTypeTraits.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTTypeTraits.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1789,5 +1789,43 @@
   EXPECT_TRUE(notMatchesWithOpenMP(Source2, Matcher));
 }
 
+TEST(OMPDefaultClause, Matches) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(ompDefaultClause()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2274,5 +2274,173 @@
 notMatches("int main2() {}", functionDecl(isMain(;
 }
 
+TEST(OMPExecutableDirective, hasClause) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(anything()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isNoneKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isNoneKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isSharedKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isSharedKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPExecutableDirective, isAllowedToContainClauseKind) {
+  auto Matcher =
+  ompExecutableDirective(isAllowedToContainClauseKind(OMPC_default));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp 

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> aaron.ballman wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > lebedev.ri wrote:
> > > > > > gribozavr wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > gribozavr wrote:
> > > > > > > > > Why not make `isAllowedToContainClause` take an 
> > > > > > > > > `OpenMPClauseKind` enum value?
> > > > > > > > > 
> > > > > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > > > > example, it can't be a more complex matcher with inner 
> > > > > > > > > matchers, it can't be a disjunction of matchers etc.)
> > > > > > > > I don't feel like it, it's uglier.
> > > > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > > > Also, how will passing some random enum work with e.g. 
> > > > > > > > clang-query?
> > > > > > > > 
> > > > > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have 
> > > > > > > to replicate them all as matchers to provide a useful API.
> > > > > > > 
> > > > > > > > Also, how will passing some random enum work with e.g. 
> > > > > > > > clang-query?
> > > > > > > 
> > > > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > > > True. Also, but there's dosens of Stmt types, and there is no 
> > > > > > overload that takes `StmtClass` enum.
> > > > > For Stmts, we do have dozens of individual matchers for them.
> > > > > 
> > > > > The point of your work is to add ASTMatchers for OpenMP, right?  
> > > > > However, if there are no matchers for a reasonable amount of AST 
> > > > > surface, it is as good as if the matchers are not there, because 
> > > > > prospective users won't be able to use them.
> > > > > 
> > > > > I don't particularly care how exactly this is achieved, through 
> > > > > individual matchers or through a matcher that takes an enum.  
> > > > > However, I want to make sure that if you're going through all this 
> > > > > trouble to add matchers, the resulting API should cover a good amount 
> > > > > of AST.
> > > > > 
> > > > > The reason why I suggested to pass the enum to the matcher is simply 
> > > > > because it is less code duplication, less work, and more reliable 
> > > > > code (since there will be only one matcher to review, test, and 
> > > > > maintain, instead of combinations of matchers).
> > > > > 
> > > > > Another reason to not use an inner matcher here is the peculiar 
> > > > > semantics of this function -- it does not evaluate the matcher, and 
> > > > > it does not accept a matcher expression of any shape.
> > > > > The point of your work is to add ASTMatchers for OpenMP, right?
> > > > 
> > > > Absolutely not.
> > > > D57113 + D59466 is the one and only point, to address the bugs i have 
> > > > personally encountered.
> > > > The whole reason why i have started off with NOT adding these matchers 
> > > > to the `ASTMatchers.h`,
> > > > but keeping them at least initially internal to the checks was to avoid 
> > > > all this bikeshedding.
> > > However, I do care about the AST matchers being usable by other clients.
> > > 
> > > I also care about the API following existing patterns:
> > > 
> > > > Another reason to not use an inner matcher here is the peculiar 
> > > > semantics of this function -- it does not evaluate the matcher, and it 
> > > > does not accept a matcher expression of any shape.
> > > 
> > > 
> > >> Also, how will passing some random enum work with e.g. clang-query?
> > > See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.
> > 
> > That doesn't mean it works super well, though. String literals more easily 
> > contain silent typos, don't have autocomplete support, etc. I can 
> > definitely sympathize with not wanting to use an enum here.
> > 
> > However, I see that there are 50+ enumerations in this list -- that seems 
> > like too many matchers to want to expose. I think an enum will be the 
> > better, more maintainable option. The current approach won't scale well.
> Okay, but apparently clang-query will needs to be fixed too:
> ```
> clang-query> match 
> stmt(ompExecutableDirective(isAllowedToContainClause(OMPC_default)))
> 1:1: Error parsing argument 1 for matcher stmt.
> 1:6: Error parsing argument 1 for matcher ompExecutableDirective.
> 1:29: Error parsing argument 1 for matcher isAllowedToContainClause.
> 1:58: Error parsing matcher. Found token <_> while looking for '('.
> 
> ```
> https://bugs.llvm.org/show_bug.cgi?id=41176
clang-query requires enumerations to be quoted string literals. If you switch 
to that in your test, does it work for you? I was spotting some odd behavior 
with a different matcher 

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@gribozavr thank you for the review!
@aaron.ballman any further comments?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

aaron.ballman wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > lebedev.ri wrote:
> > > > > gribozavr wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > gribozavr wrote:
> > > > > > > > Why not make `isAllowedToContainClause` take an 
> > > > > > > > `OpenMPClauseKind` enum value?
> > > > > > > > 
> > > > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > > > example, it can't be a more complex matcher with inner 
> > > > > > > > matchers, it can't be a disjunction of matchers etc.)
> > > > > > > I don't feel like it, it's uglier.
> > > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > > Also, how will passing some random enum work with e.g. 
> > > > > > > clang-query?
> > > > > > > 
> > > > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have 
> > > > > > to replicate them all as matchers to provide a useful API.
> > > > > > 
> > > > > > > Also, how will passing some random enum work with e.g. 
> > > > > > > clang-query?
> > > > > > 
> > > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > > True. Also, but there's dosens of Stmt types, and there is no 
> > > > > overload that takes `StmtClass` enum.
> > > > For Stmts, we do have dozens of individual matchers for them.
> > > > 
> > > > The point of your work is to add ASTMatchers for OpenMP, right?  
> > > > However, if there are no matchers for a reasonable amount of AST 
> > > > surface, it is as good as if the matchers are not there, because 
> > > > prospective users won't be able to use them.
> > > > 
> > > > I don't particularly care how exactly this is achieved, through 
> > > > individual matchers or through a matcher that takes an enum.  However, 
> > > > I want to make sure that if you're going through all this trouble to 
> > > > add matchers, the resulting API should cover a good amount of AST.
> > > > 
> > > > The reason why I suggested to pass the enum to the matcher is simply 
> > > > because it is less code duplication, less work, and more reliable code 
> > > > (since there will be only one matcher to review, test, and maintain, 
> > > > instead of combinations of matchers).
> > > > 
> > > > Another reason to not use an inner matcher here is the peculiar 
> > > > semantics of this function -- it does not evaluate the matcher, and it 
> > > > does not accept a matcher expression of any shape.
> > > > The point of your work is to add ASTMatchers for OpenMP, right?
> > > 
> > > Absolutely not.
> > > D57113 + D59466 is the one and only point, to address the bugs i have 
> > > personally encountered.
> > > The whole reason why i have started off with NOT adding these matchers to 
> > > the `ASTMatchers.h`,
> > > but keeping them at least initially internal to the checks was to avoid 
> > > all this bikeshedding.
> > However, I do care about the AST matchers being usable by other clients.
> > 
> > I also care about the API following existing patterns:
> > 
> > > Another reason to not use an inner matcher here is the peculiar semantics 
> > > of this function -- it does not evaluate the matcher, and it does not 
> > > accept a matcher expression of any shape.
> > 
> > 
> >> Also, how will passing some random enum work with e.g. clang-query?
> > See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.
> 
> That doesn't mean it works super well, though. String literals more easily 
> contain silent typos, don't have autocomplete support, etc. I can definitely 
> sympathize with not wanting to use an enum here.
> 
> However, I see that there are 50+ enumerations in this list -- that seems 
> like too many matchers to want to expose. I think an enum will be the better, 
> more maintainable option. The current approach won't scale well.
Okay, but apparently clang-query will needs to be fixed too:
```
clang-query> match 
stmt(ompExecutableDirective(isAllowedToContainClause(OMPC_default)))
1:1: Error parsing argument 1 for matcher stmt.
1:6: Error parsing argument 1 for matcher ompExecutableDirective.
1:29: Error parsing argument 1 for matcher isAllowedToContainClause.
1:58: Error parsing matcher. Found token <_> while looking for '('.

```
https://bugs.llvm.org/show_bug.cgi?id=41176



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

aaron.ballman wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > lebedev.ri wrote:
> > > > > gribozavr wrote:
> > > > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > 

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 191678.
lebedev.ri marked 18 inline comments as done.
lebedev.ri added a comment.

Rebased, addressed all(?) nits.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112

Files:
  docs/LibASTMatchersReference.html
  include/clang/AST/ASTTypeTraits.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTTypeTraits.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1789,5 +1789,43 @@
   EXPECT_TRUE(notMatchesWithOpenMP(Source2, Matcher));
 }
 
+TEST(OMPDefaultClause, Matches) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(ompDefaultClause()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2274,5 +2274,173 @@
 notMatches("int main2() {}", functionDecl(isMain(;
 }
 
+TEST(OMPExecutableDirective, hasClause) {
+  auto Matcher = ompExecutableDirective(hasAnyClause(anything()));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isNoneKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isNoneKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isSharedKind) {
+  auto Matcher =
+  ompExecutableDirective(hasAnyClause(ompDefaultClause(isSharedKind(;
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(
+void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(
+void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(
+void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPExecutableDirective, isAllowedToContainClauseKind) {
+  auto Matcher =
+  ompExecutableDirective(isAllowedToContainClauseKind(OMPC_default));
+
+  const std::string Source0 = R"(
+void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(
+void x() {
+#pragma omp parallel

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher,
+  InnerMatcher) {

gribozavr wrote:
> Looking at other similar matches, they usually follow the naming pattern 
> `hasAny~`, WDYT?
> 
> (`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)
Yeah, I think this would be a good change to make. Good catch!



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+  return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+

gribozavr wrote:
> Also add `isSharedKind`?  It is weird that we have one but not the other.
Given that there's only two options, it's not strictly required to have them 
both. But I don't see a reason not to add it, either.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > lebedev.ri wrote:
> > > > > > gribozavr wrote:
> > > > > > > Why not make `isAllowedToContainClause` take an 
> > > > > > > `OpenMPClauseKind` enum value?
> > > > > > > 
> > > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > > example, it can't be a more complex matcher with inner matchers, 
> > > > > > > it can't be a disjunction of matchers etc.)
> > > > > > I don't feel like it, it's uglier.
> > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > > 
> > > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > > > replicate them all as matchers to provide a useful API.
> > > > > 
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > 
> > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > True. Also, but there's dosens of Stmt types, and there is no overload 
> > > > that takes `StmtClass` enum.
> > > For Stmts, we do have dozens of individual matchers for them.
> > > 
> > > The point of your work is to add ASTMatchers for OpenMP, right?  However, 
> > > if there are no matchers for a reasonable amount of AST surface, it is as 
> > > good as if the matchers are not there, because prospective users won't be 
> > > able to use them.
> > > 
> > > I don't particularly care how exactly this is achieved, through 
> > > individual matchers or through a matcher that takes an enum.  However, I 
> > > want to make sure that if you're going through all this trouble to add 
> > > matchers, the resulting API should cover a good amount of AST.
> > > 
> > > The reason why I suggested to pass the enum to the matcher is simply 
> > > because it is less code duplication, less work, and more reliable code 
> > > (since there will be only one matcher to review, test, and maintain, 
> > > instead of combinations of matchers).
> > > 
> > > Another reason to not use an inner matcher here is the peculiar semantics 
> > > of this function -- it does not evaluate the matcher, and it does not 
> > > accept a matcher expression of any shape.
> > > The point of your work is to add ASTMatchers for OpenMP, right?
> > 
> > Absolutely not.
> > D57113 + D59466 is the one and only point, to address the bugs i have 
> > personally encountered.
> > The whole reason why i have started off with NOT adding these matchers to 
> > the `ASTMatchers.h`,
> > but keeping them at least initially internal to the checks was to avoid all 
> > this bikeshedding.
> However, I do care about the AST matchers being usable by other clients.
> 
> I also care about the API following existing patterns:
> 
> > Another reason to not use an inner matcher here is the peculiar semantics 
> > of this function -- it does not evaluate the matcher, and it does not 
> > accept a matcher expression of any shape.
> 
> 
>> Also, how will passing some random enum work with e.g. clang-query?
> See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.

That doesn't mean it works super well, though. String literals more easily 
contain silent typos, don't have autocomplete support, etc. I can definitely 
sympathize with not wanting to use an enum here.

However, I see that there are 50+ enumerations in this list -- that seems like 
too many matchers to want to expose. I think an enum will be the better, more 
maintainable option. The current approach won't scale well.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> lebedev.ri wrote:
> > 

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > lebedev.ri wrote:
> > > > > gribozavr wrote:
> > > > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` 
> > > > > > enum value?
> > > > > > 
> > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > example, it can't be a more complex matcher with inner matchers, it 
> > > > > > can't be a disjunction of matchers etc.)
> > > > > I don't feel like it, it's uglier.
> > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > 
> > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > > replicate them all as matchers to provide a useful API.
> > > > 
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > 
> > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > True. Also, but there's dosens of Stmt types, and there is no overload 
> > > that takes `StmtClass` enum.
> > For Stmts, we do have dozens of individual matchers for them.
> > 
> > The point of your work is to add ASTMatchers for OpenMP, right?  However, 
> > if there are no matchers for a reasonable amount of AST surface, it is as 
> > good as if the matchers are not there, because prospective users won't be 
> > able to use them.
> > 
> > I don't particularly care how exactly this is achieved, through individual 
> > matchers or through a matcher that takes an enum.  However, I want to make 
> > sure that if you're going through all this trouble to add matchers, the 
> > resulting API should cover a good amount of AST.
> > 
> > The reason why I suggested to pass the enum to the matcher is simply 
> > because it is less code duplication, less work, and more reliable code 
> > (since there will be only one matcher to review, test, and maintain, 
> > instead of combinations of matchers).
> > 
> > Another reason to not use an inner matcher here is the peculiar semantics 
> > of this function -- it does not evaluate the matcher, and it does not 
> > accept a matcher expression of any shape.
> > The point of your work is to add ASTMatchers for OpenMP, right?
> 
> Absolutely not.
> D57113 + D59466 is the one and only point, to address the bugs i have 
> personally encountered.
> The whole reason why i have started off with NOT adding these matchers to the 
> `ASTMatchers.h`,
> but keeping them at least initially internal to the checks was to avoid all 
> this bikeshedding.
However, I do care about the AST matchers being usable by other clients.

I also care about the API following existing patterns:

> Another reason to not use an inner matcher here is the peculiar semantics of 
> this function -- it does not evaluate the matcher, and it does not accept a 
> matcher expression of any shape.




Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` 
> > > > > enum value?
> > > > > 
> > > > > I don't see right now advantages for taking a matcher.  (For example, 
> > > > > it can't be a more complex matcher with inner matchers, it can't be a 
> > > > > disjunction of matchers etc.)
> > > > I don't feel like it, it's uglier.
> > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > 
> > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > replicate them all as matchers to provide a useful API.
> > > 
> > > > Also, how will passing some random enum work with e.g. clang-query?
> > > 
> > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > True. Also, but there's dosens of Stmt types, and there is no overload that 
> > takes `StmtClass` enum.
> For Stmts, we do have dozens of individual matchers for them.
> 
> The point of your work is to add ASTMatchers for OpenMP, right?  However, if 
> there are no matchers for a reasonable amount of AST surface, it is as good 
> as if the matchers are not there, because prospective users won't be able to 
> use them.
> 
> I don't particularly care how exactly this is achieved, through individual 
> matchers or through a matcher that takes an enum.  However, I want to make 
> sure that if you're going through all this trouble to add matchers, the 
> resulting API should cover a good amount of AST.
> 
> The reason why I suggested to pass the enum to the matcher is simply because 
> it is less code duplication, less work, and more reliable code (since there 
> will be only one matcher to review, test, and maintain, instead of 
> combinations of matchers).
> 
> Another reason to not use an inner matcher here is the peculiar semantics of 
> this function -- it does not evaluate the matcher, and it does not accept a 
> matcher expression of any shape.
> The point of your work is to add ASTMatchers for OpenMP, right?

Absolutely not.
D57113 + D59466 is the one and only point, to address the bugs i have 
personally encountered.
The whole reason why i have started off with NOT adding these matchers to the 
`ASTMatchers.h`,
but keeping them at least initially internal to the checks was to avoid all 
this bikeshedding.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > > > value?
> > > > 
> > > > I don't see right now advantages for taking a matcher.  (For example, 
> > > > it can't be a more complex matcher with inner matchers, it can't be a 
> > > > disjunction of matchers etc.)
> > > I don't feel like it, it's uglier.
> > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > Also, how will passing some random enum work with e.g. clang-query?
> > > 
> > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > replicate them all as matchers to provide a useful API.
> > 
> > > Also, how will passing some random enum work with e.g. clang-query?
> > 
> > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> True. Also, but there's dosens of Stmt types, and there is no overload that 
> takes `StmtClass` enum.
For Stmts, we do have dozens of individual matchers for them.

The point of your work is to add ASTMatchers for OpenMP, right?  However, if 
there are no matchers for a reasonable amount of AST surface, it is as good as 
if the matchers are not there, because prospective users won't be able to use 
them.

I don't particularly care how exactly this is achieved, through individual 
matchers or through a matcher that takes an enum.  However, I want to make sure 
that if you're going through all this trouble to add matchers, the resulting 
API should cover a good amount of AST.

The reason why I suggested to pass the enum to the matcher is simply because it 
is less code duplication, less work, and more reliable code (since there will 
be only one matcher to review, test, and maintain, instead of combinations of 
matchers).

Another reason to not use an inner matcher here is the peculiar semantics of 
this function -- it does not evaluate the matcher, and it does not accept a 
matcher expression of any shape.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability.  WDYT about inlining the code into the 
> > > > EXPECT_TRUE code like in other tests in this file?
> > > > 
> > > > If you want to break it out, I'd suggest to drop "`void x() {`" down to 
> > > > the next line, so that all code lines start at the same column.
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability
> > > 
> > > It's not about readability. Inlining will break the build, rC354201.
> > Other tests in this file use string concatenation, see 
> > `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
> I'm sorry, but i fail to see how that is relevant?
> I'm using multiline raw string literals, and inlining it will break the 
> build, like i linked.
> You are pointing at the code that is not using multiline raw string literals.
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?
> 
> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?

I am now suggesting to switch away from raw string literals.

> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"

I believe that adding "\n" manually is better than having lots of 
similarly-named SourceX variables, which can easily cause copy-paste mistakes 
(define a SourceX variable, use SourceY in the EXPECT_TRUE line).

However, this is a minor point, up to you.  I only wanted to explain my 
reasoning why I prefer inline code snippets.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 5 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > > value?
> > > 
> > > I don't see right now advantages for taking a matcher.  (For example, it 
> > > can't be a more complex matcher with inner matchers, it can't be a 
> > > disjunction of matchers etc.)
> > I don't feel like it, it's uglier.
> > The matcher is documented, `OpenMPClauseKind` is not documented.
> > Also, how will passing some random enum work with e.g. clang-query?
> > 
> There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> replicate them all as matchers to provide a useful API.
> 
> > Also, how will passing some random enum work with e.g. clang-query?
> 
> See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
True. Also, but there's dosens of Stmt types, and there is no overload that 
takes `StmtClass` enum.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > I'm not sure if breaking out the source code into the "SourceX" variables 
> > > improves readability.  WDYT about inlining the code into the EXPECT_TRUE 
> > > code like in other tests in this file?
> > > 
> > > If you want to break it out, I'd suggest to drop "`void x() {`" down to 
> > > the next line, so that all code lines start at the same column.
> > > I'm not sure if breaking out the source code into the "SourceX" variables 
> > > improves readability
> > 
> > It's not about readability. Inlining will break the build, rC354201.
> Other tests in this file use string concatenation, see 
> `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
I'm sorry, but i fail to see how that is relevant?
I'm using multiline raw string literals, and inlining it will break the build, 
like i linked.
You are pointing at the code that is not using multiline raw string literals.
You only suggested inlining, not switching away from multiline raw string 
literals, i believe?

Not using multiline raw string literals looked even worse, because then you 
need to manually add "\n"


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > value?
> > 
> > I don't see right now advantages for taking a matcher.  (For example, it 
> > can't be a more complex matcher with inner matchers, it can't be a 
> > disjunction of matchers etc.)
> I don't feel like it, it's uglier.
> The matcher is documented, `OpenMPClauseKind` is not documented.
> Also, how will passing some random enum work with e.g. clang-query?
> 
There are dozens of clauses in `OpenMPClauseKind`.  We would have to replicate 
them all as matchers to provide a useful API.

> Also, how will passing some random enum work with e.g. clang-query?

See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability.  WDYT about inlining the code into the EXPECT_TRUE 
> > code like in other tests in this file?
> > 
> > If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
> > next line, so that all code lines start at the same column.
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability
> 
> It's not about readability. Inlining will break the build, rC354201.
Other tests in this file use string concatenation, see 
`TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 4 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum value?
> 
> I don't see right now advantages for taking a matcher.  (For example, it 
> can't be a more complex matcher with inner matchers, it can't be a 
> disjunction of matchers etc.)
I don't feel like it, it's uglier.
The matcher is documented, `OpenMPClauseKind` is not documented.
Also, how will passing some random enum work with e.g. clang-query?




Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> I'm not sure if breaking out the source code into the "SourceX" variables 
> improves readability.  WDYT about inlining the code into the EXPECT_TRUE code 
> like in other tests in this file?
> 
> If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
> next line, so that all code lines start at the same column.
> I'm not sure if breaking out the source code into the "SourceX" variables 
> improves readability

It's not about readability. Inlining will break the build, rC354201.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6390
+/// Given OpenMP directive, matches the first clause (out of all specified),
+/// that matches InnerMatcher.
+///

Other comments usually don't explain the interactions with inner matchers, 
unless the semantics are unusual.

"Matches any clause in an OpenMP directive."



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher,
+  InnerMatcher) {

Looking at other similar matches, they usually follow the naming pattern 
`hasAny~`, WDYT?

(`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6411
+///
+/// Given, `ompDefaultClause()`:
+///

Please follow the existing comment style.  Either:

```
Given

\code

\endcode

 matches "".
```

or

```
Example:  matches "" in 

\code

\endcode   
```

For example:

```
Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.
```

Similarly for other comments in this patch.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+  return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+

Also add `isSharedKind`?  It is weird that we have one but not the other.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6449
+///
+/// NOTE: while the matcher takes the *matcher* for the OpenMP clause,
+///   it does *NOT* actually match that matcher. It only fetches the

"while this matcher"



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum value?

I don't see right now advantages for taking a matcher.  (For example, it can't 
be a more complex matcher with inner matchers, it can't be a disjunction of 
matchers etc.)



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

I'm not sure if breaking out the source code into the "SourceX" variables 
improves readability.  WDYT about inlining the code into the EXPECT_TRUE code 
like in other tests in this file?

If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
next line, so that all code lines start at the same column.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTTypeTraits.h:81
 
+  /// \{
+  /// Return the AST node kind of this ASTNodeKind.

lebedev.ri wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > These markings are a bit strange, can you explain them to me?
> > > It is weird, but i think this is the right solution.
> > > See `isAllowedToContainClause()` narrower.
> > > This `asOMPClauseKind()` allows to pass a whole matcher, and then distill 
> > > the inner output node type.
> > > I.e. now i can spell 
> > > `ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()))`
> > > (and the `ompDefaultClause()` won't actually be used for matching!), 
> > > instead of doing something like e.g.
> > > `ompExecutableDirective(isAllowedToContainClause(OMPC_default))`
> > > which looks horrible, and will likely not work well with `clang-query`?
> > I think we may be talking about different things. I only meant the `\{` and 
> > `\}` comment markers. :-D I think the design is reasonable enough, but I'm 
> > not an OpenMP expert. Truth be told, I was mostly surprised that these 
> > pragmas will have corresponding AST matchers. I thought they were 
> > preprocessor directives and thus would be handled at that level, rather 
> > than the AST level. My only concern about the design of this is a pretty 
> > minor one: what happens if someone is using preprocessor callbacks and AST 
> > matchers at the same time -- will they get duplicate results for OpenMP 
> > directives? I suspect they will, but that doesn't mean we shouldn't AST 
> > match OpenMP clauses (they are in the AST, after all).
> Oh duh xD
> I just copied them from around the `getFromNode()` up there ^.
> I believe they signify that these functions should be displayed as a group in 
> doxygen.
> 
> I don't know what will happen when using preprocessor callbacks and AST 
> matchers at the same time,
> but i //think// that would be the issue of the user in question.
> I just copied them from around the getFromNode() up there ^.
> I believe they signify that these functions should be displayed as a group in 
> doxygen.

I thought so as well, but there's no grouping here. I'd probably drop them 
until it's needed.

> I don't know what will happen when using preprocessor callbacks and AST 
> matchers at the same time, but i think that would be the issue of the user in 
> question.

I agree.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/AST/ASTTypeTraits.h:81
 
+  /// \{
+  /// Return the AST node kind of this ASTNodeKind.

aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > These markings are a bit strange, can you explain them to me?
> > It is weird, but i think this is the right solution.
> > See `isAllowedToContainClause()` narrower.
> > This `asOMPClauseKind()` allows to pass a whole matcher, and then distill 
> > the inner output node type.
> > I.e. now i can spell 
> > `ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()))`
> > (and the `ompDefaultClause()` won't actually be used for matching!), 
> > instead of doing something like e.g.
> > `ompExecutableDirective(isAllowedToContainClause(OMPC_default))`
> > which looks horrible, and will likely not work well with `clang-query`?
> I think we may be talking about different things. I only meant the `\{` and 
> `\}` comment markers. :-D I think the design is reasonable enough, but I'm 
> not an OpenMP expert. Truth be told, I was mostly surprised that these 
> pragmas will have corresponding AST matchers. I thought they were 
> preprocessor directives and thus would be handled at that level, rather than 
> the AST level. My only concern about the design of this is a pretty minor 
> one: what happens if someone is using preprocessor callbacks and AST matchers 
> at the same time -- will they get duplicate results for OpenMP directives? I 
> suspect they will, but that doesn't mean we shouldn't AST match OpenMP 
> clauses (they are in the AST, after all).
Oh duh xD
I just copied them from around the `getFromNode()` up there ^.
I believe they signify that these functions should be displayed as a group in 
doxygen.

I don't know what will happen when using preprocessor callbacks and AST 
matchers at the same time,
but i //think// that would be the issue of the user in question.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTTypeTraits.h:81
 
+  /// \{
+  /// Return the AST node kind of this ASTNodeKind.

lebedev.ri wrote:
> aaron.ballman wrote:
> > These markings are a bit strange, can you explain them to me?
> It is weird, but i think this is the right solution.
> See `isAllowedToContainClause()` narrower.
> This `asOMPClauseKind()` allows to pass a whole matcher, and then distill the 
> inner output node type.
> I.e. now i can spell 
> `ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()))`
> (and the `ompDefaultClause()` won't actually be used for matching!), instead 
> of doing something like e.g.
> `ompExecutableDirective(isAllowedToContainClause(OMPC_default))`
> which looks horrible, and will likely not work well with `clang-query`?
I think we may be talking about different things. I only meant the `\{` and 
`\}` comment markers. :-D I think the design is reasonable enough, but I'm not 
an OpenMP expert. Truth be told, I was mostly surprised that these pragmas will 
have corresponding AST matchers. I thought they were preprocessor directives 
and thus would be handled at that level, rather than the AST level. My only 
concern about the design of this is a pretty minor one: what happens if someone 
is using preprocessor callbacks and AST matchers at the same time -- will they 
get duplicate results for OpenMP directives? I suspect they will, but that 
doesn't mean we shouldn't AST match OpenMP clauses (they are in the AST, after 
all).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.



Comment at: include/clang/AST/ASTTypeTraits.h:81
 
+  /// \{
+  /// Return the AST node kind of this ASTNodeKind.

aaron.ballman wrote:
> These markings are a bit strange, can you explain them to me?
It is weird, but i think this is the right solution.
See `isAllowedToContainClause()` narrower.
This `asOMPClauseKind()` allows to pass a whole matcher, and then distill the 
inner output node type.
I.e. now i can spell 
`ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()))`
(and the `ompDefaultClause()` won't actually be used for matching!), instead of 
doing something like e.g.
`ompExecutableDirective(isAllowedToContainClause(OMPC_default))`
which looks horrible, and will likely not work well with `clang-query`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTTypeTraits.h:81
 
+  /// \{
+  /// Return the AST node kind of this ASTNodeKind.

These markings are a bit strange, can you explain them to me?



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6389
 
+/// Given OpenMP directive, matches the first clause (out of all specified),
+/// that matches InnerMatcher.

Given OpenMP -> Given an OpenMP



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:509-512
+  REGISTER_MATCHER(hasClause);
+  REGISTER_MATCHER(ompDefaultClause);
+  REGISTER_MATCHER(isNoneKind);
+  REGISTER_MATCHER(isAllowedToContainClause);

Please keep these alphabetically sorted.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 190965.
lebedev.ri marked 3 inline comments as done.
lebedev.ri retitled this revision from "[ASTTypeTraits] OMPClause handling" to 
"[ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling".
lebedev.ri edited the summary of this revision.
lebedev.ri added reviewers: aaron.ballman, george.karpenkov.
lebedev.ri added a comment.
Herald added a subscriber: guansong.

Split off matchers from from D57113 , added 
tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112

Files:
  docs/LibASTMatchersReference.html
  include/clang/AST/ASTTypeTraits.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTTypeTraits.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1780,5 +1780,38 @@
   EXPECT_TRUE(notMatchesWithOpenMP(SourceBad, Matcher));
 }
 
+TEST(OMPDefaultClause, Matches) {
+  auto Matcher = ompExecutableDirective(hasClause(ompDefaultClause()));
+
+  const std::string Source0 = R"(void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2274,5 +2274,117 @@
 notMatches("int main2() {}", functionDecl(isMain(;
 }
 
+TEST(OMPExecutableDirective, hasClause) {
+  auto Matcher = ompExecutableDirective(hasClause(anything()));
+
+  const std::string Source0 = R"(void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPDefaultClause, isNoneKind) {
+  auto Matcher =
+  ompExecutableDirective(hasClause(ompDefaultClause(isNoneKind(;
+
+  const std::string Source0 = R"(void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+}
+
+TEST(OMPExecutableDirective, isAllowedToContainClause) {
+  auto Matcher =
+  ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()));
+
+  const std::string Source0 = R"(void x() {
+;
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
+  const std::string Source1 = R"(void x() {
+#pragma omp parallel
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source1, Matcher));
+
+  const std::string Source2 = R"(void x() {
+#pragma omp parallel default(none)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source2, Matcher));
+
+  const std::string Source3 = R"(void x() {
+#pragma omp parallel default(shared)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
+
+  const std::string Source4 = R"(void x(int x) {
+#pragma omp parallel num_threads(x)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+
+  const std::string Source5 = R"(void x() {
+#pragma omp taskyield
+})";
+