[PATCH] D57112: [ASTTypeTraits] OMPClause handling

Mon Mar 4 03:25:45 PST 2019

gribozavr added a comment.

> The test coverage i have will be in the clang-tidy check.

Please add unit tests to `clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp`.  You can pull matchers from D57113 <https://reviews.llvm.org/D57113> into the shared matchers library (instead of keeping them private to the check), and add unit tests.

================
Comment at: include/clang/AST/ASTTypeTraits.h:83
+  /// Return the AST node kind of this ASTNodeKind.
+  OpenMPClauseKind getOMPClauseKind() const;
+  /// \}
----------------
"asOMPClauseKind()"?

================
Comment at: lib/AST/ASTTypeTraits.cpp:114
+#define OPENMP_CLAUSE(Name, Class)                                             \
+    case OMPC_##Name: return ASTNodeKind(NKI_##Class);
+#include "clang/Basic/OpenMPKinds.def"
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > lebedev.ri wrote:
> > > > > ABataev wrote:
> > > > > > Well, I think it would be good to filter out `OMPC_flush` somehow because there is no such clause actually, it is a pseudo clause for better handling of the `flush` directive.
> > > > > > 
> > > > > Are `OMPC_threadprivate` and `OMPC_uniform` also in the same boat?
> > > > > I don't see those clauses in spec.
> > > > > 
> > > > > Perhaps `OMPC_flush` should be made more like those other two?
> > > > > (i.e. handled outside of `OPENMP_CLAUSE` macro)
> > > > `OMPC_threadprivate` is also a special kind of pseudo-clause.
> > > > `OMPC_flush` is in the enum, because it has the corresponding class. You can try to exclude it from the enum, but it may require some additional work.
> > > > `OMPC_uniform` is a normal clause, but it has the corresponding class. This clause can be used on `declare simd` directive, which is represented as an attribute.
> > > I mean, `OOMPC_uniform` has no(!) corresponding class. Mistyped
> > As one would expect, simply adding 
> > ```
> >   case OMPC_flush: // Pseudo clause, does not exist (keep before including .def)
> >     llvm_unreachable("unexpected OpenMP clause kind");
> > ```
> > results in a
> > ```
> > [58/1118 5.6/sec] Building CXX object tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o
> > FAILED: tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o 
> > /usr/bin/g++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/AST -I/build/clang/lib/AST -I/build/clang/include -Itools/clang/include -I/usr/include/libxml2 -Iinclude -I/build/llvm/include -pipe -O2 -g0 -UNDEBUG -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -pipe -O2 -g0 -UNDEBUG -fPIC   -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o -MF tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o.d -o tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o -c /build/clang/lib/AST/ASTTypeTraits.cpp
> > /build/clang/include/clang/Basic/OpenMPKinds.def: In static member function ‘static clang::ast_type_traits::ASTNodeKind clang::ast_type_traits::ASTNodeKind::getFromNode(const clang::OMPClause&)’:
> > /build/clang/lib/AST/ASTTypeTraits.cpp:116:5: error: duplicate case value
> >      case OMPC_##Name: return ASTNodeKind(NKI_##Class);
> >      ^~~~
> > /build/clang/include/clang/Basic/OpenMPKinds.def:261:1: note: in expansion of macro ‘OPENMP_CLAUSE’
> >  OPENMP_CLAUSE(flush, OMPFlushClause)
> >  ^~~~~~~~~~~~~
> > /build/clang/lib/AST/ASTTypeTraits.cpp:113:3: note: previously used here
> >    case OMPC_flush: // Pseudo clause, does not exist (keep before including .def)
> >    ^~~~
> > ```
> > So one will need to pull `OMPC_flush` out of `clang/Basic/OpenMPKinds.def`.
> D57280, will rebase this.
> Well, I think it would be good to filter out `OMPC_flush` somehow because there is no such clause actually, it is a pseudo clause for better handling of the `flush` directive.

Sorry to be late for this discussion, but I don't think this conclusion follows.  ASTMatchers are supposed to match the AST as it is.  Even if `OMPC_flush` is synthetic, it exists in the AST, and users might want to match it.  I think users would find anything else (trying to filter out AST nodes that are not in the source code) to be surprising. For example, there's a matcher `materializeTemporaryExpr` even though this AST node is a Clang invention and is not a part of the C++ spec.

Matching only constructs that appear in the source code is not feasible with ASTMatchers, because they are based on Clang's AST that exposes tons of semantic information, and its design is dictated by the structure of the semantic information.  See "RFC: Tree-based refactorings with Clang" in cfe-dev for a library that will focus on representing source code as faithfully as possible.

Not to even mention that this code is in ASTTypeTraits, a general library for handling AST nodes, not specifically for AST Matchers...

================
Comment at: lib/AST/ASTTypeTraits.cpp:41
+  { NKI_None, "OMPClause" },
+#define OPENMP_CLAUSE(TextualSpelling, DERIVED) { NKI_OMPClause, #DERIVED },
+#include "clang/Basic/OpenMPKinds.def"
----------------
Why "DERIVED"?  The definition of `OPENMP_CLAUSE` calls the second argument "Class" which I think is more appropriate.

================
Comment at: lib/AST/ASTTypeTraits.cpp:70
+    llvm_unreachable("Unknown clause kind!");
+#define OPENMP_CLAUSE(TextualSpelling, DERIVED)                                \
+  case NKI_##DERIVED:                                                          \
----------------
Why "DERIVED"?  The definition of `OPENMP_CLAUSE` calls the second argument "Class" which I think is more appropriate.

Repository:
  rC Clang

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

https://reviews.llvm.org/D57112