jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.
In D71830#1854989 <https://reviews.llvm.org/D71830#1854989>, @kiranchandramohan
wrote:
> Had a quick look today. Will spend some more time tomorrow.
> Please see a few comments inline.
Thanks! I'll rebase asap to address your comments.
================
Comment at: clang/include/clang/AST/OpenMPClause.h:6429
+/// and an ordered collection of properties.
+struct OpenMPTraitInfo {
+ struct OpenMPTraitProperty {
----------------
kiranchandramohan wrote:
> Not clear from this file what should be named starting with OMP or OpenMP. I
> see more classes/structs strating with OMP.
I will go for OMP everywhere.
================
Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272
+
+ template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); }
+
----------------
kiranchandramohan wrote:
> Compiler throws up this error.
> error: explicit specialization in non-namespace scope ‘class
> clang::ASTRecordReader’
Oh, my compiler was happy. Let me rebase and see what the pre-merge bots say so
I might get some insight into the problem.
================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:277
+
+ template <> void writeUserType(OpenMPTraitInfo *TI) {
+ writeOpenMPTraitInfo(TI);
----------------
kiranchandramohan wrote:
> Compiler throws up this error.
> error: explicit specialization in non-namespace scope ‘class
> clang::ASTRecordWriter’
Same as above I guess.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:853
+ llvm::omp::TraitSelector Selector, llvm::StringMap<SourceLocation> &Seen) {
+ const unsigned Lvl = 2;
+ TIProperty.Kind = TraitProperty::invalid;
----------------
kiranchandramohan wrote:
> Would it be better to add the value of this variable also to the name? Like
> Lvl_TWO or something? Same question for the 5 other const Lvl variables in
> this patch.
Yes, that would be better. Will fix.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71830/new/
https://reviews.llvm.org/D71830
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
