[PATCH] D122255: Meta directive runtime support

[Abid via Phabricator via cfe-commits]

abidmalikwaterloo marked 4 inline comments as done.
abidmalikwaterloo added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10852
+def err_omp_misplaced_default_clause : Error<
+  "misplaced default clause! Only one default clause is allowed in"
+  "metadirective in the end">;
----------------
jdoerfert wrote:
> ABataev wrote:
> > We do not use explamations in messages, better to have something `only single default ...`
> "allowed in" what? Also, there is no test for this, or is there?
It should be "Only one default clause is allowed.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1679
+  OS << ": ";
+}
+
----------------
jdoerfert wrote:
> I'm assuming we already have a printer for trait selectors, no? Doesn't `OMPTraitInfo::print` do this already and actually handle scores?
Looked into the function. `OMPTraitInfo::print` can be used. The function needs to be extended as well to take the other traits as well.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7439
+      }
+    }
+
----------------
jdoerfert wrote:
> Why does this perform partial trait matching? We should have code for this. Also, the logic for device_arch and vendor (which is most what there is), is not what we want. Reuse the existing matching logic instead.
Ok. What do you mean by `existing matching logic`? 


================
Comment at: clang/lib/Sema/SemaStmt.cpp:4795
+  		
+  CD->setBody(Res->getCapturedStmt());   
   RD->completeDefinition();
----------------
jdoerfert wrote:
> Unrelated. Please go over the patch and avoid these in the future. We have so many comments that point out these things that now the comments make it hard to read/review. 
An accidental tap. Removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122255