lebedev.ri marked 6 inline comments as not done.
lebedev.ri added inline comments.
================
Comment at: lib/AST/ASTTypeTraits.cpp:114
+#define OPENMP_CLAUSE(Name, Class)
\
+ case OMPC_##Name: return ASTNodeKind(NKI_##Class);
+#include "clang/Basic/OpenMPKinds.def"
----------------
gribozavr wrote:
> 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...
So, uh, i should have checked beforehand.
https://godbolt.org/z/aanQ8U
D57280 is unjustified (and thus incorrect) and **needs** to be reverted.
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
