[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<OMPClause>,
+              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
<code snippet>
\endcode

<matcher expression> matches "<code snippet>".
```

or

```
Example: <matcher expression> matches "<code snippet>" in 

\code
<code snippet>
\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