[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Johannes Doerfert via Phabricator via llvm-commits]

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