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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Had a quick look today. Will spend some more time tomorrow.
Please see a few comments inline.



================
Comment at: clang/include/clang/AST/OpenMPClause.h:6429
+/// and an ordered collection of properties.
+struct OpenMPTraitInfo {
+  struct OpenMPTraitProperty {
----------------
Not clear from this file what should be named starting with OMP or OpenMP. I see more classes/structs strating with OMP.


================
Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272
+
+  template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); }
+
----------------
Compiler throws up this error.
error: explicit specialization in non-namespace scope ‘class clang::ASTRecordReader’


================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:277
+
+  template <> void writeUserType(OpenMPTraitInfo *TI) {
+    writeOpenMPTraitInfo(TI);
----------------
Compiler throws up this error.
error: explicit specialization in non-namespace scope ‘class clang::ASTRecordWriter’


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:853
+    llvm::omp::TraitSelector Selector, llvm::StringMap<SourceLocation> &Seen) {
+  const unsigned Lvl = 2;
+  TIProperty.Kind = TraitProperty::invalid;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830