[PATCH] OpenMP threadprivate directive

Doug Gregor dgregor at apple.com
Fri Mar 8 14:01:40 PST 2013


  This is looking really good. With a few minor changes noted above, it's ready to go in. Thanks!


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:775
@@ +774,3 @@
+def warn_omp_extra_tokens_at_eol : Warning <
+  "extra tokens at end of '#pragma omp %0' - ignored">,
+  InGroup<ExtraTokens>;
----------------
Instead of "- ignored", I suggest "are ignored", which is more in line with other "ignored" diagnostics in Clang.

================
Comment at: lib/AST/DeclOpenMP.cpp:56
@@ +55,3 @@
+void OMPThreadPrivateDecl::setVars(ArrayRef<DeclRefExpr *> VL) {
+  assert((NumVars == 0 || VL.size() == NumVars) &&
+         "Number of variables is not the same as the preallocated buffer");
----------------
Why the "NumVars == 0" case here? This would mean that VL could have some variable references in it, but when NumVars==0, those variable references would be silently dropped. 

================
Comment at: lib/AST/DeclOpenMP.cpp:58
@@ +57,3 @@
+         "Number of variables is not the same as the preallocated buffer");
+  if (NumVars > 0) {
+    DeclRefExpr **Vars = reinterpret_cast<DeclRefExpr **>(this + 1);
----------------
We don't really need the "if" here. std::copy() can handle the 0 case fine.

================
Comment at: lib/Sema/SemaOpenMP.cpp:72
@@ +71,3 @@
+         << FixItHint::CreateReplacement(I->getLoc(), CorrectedStr);
+      continue;
+    }
----------------
We shouldn't unconditionally continue here. If we've found a correction, we should set VD to that correction at proceed as if the user had written the name of VD.

================
Comment at: lib/Sema/SemaOpenMP.cpp:152
@@ +151,3 @@
+    if (VD->getType()->isReferenceType()) {
+      Diag(ILoc, diag::err_omp_ref_type_arg)
+        << getOpenMPDirectiveName(OMPD_threadprivate);
----------------
Should this diagnostic include the type? It might not be obvious what the type is if it we're inside a template instantiation.

================
Comment at: lib/Sema/SemaOpenMP.cpp:148
@@ +147,3 @@
+    // OpenMP [2.9.2, Restrictions, C/C++, p.9]
+    //   The address of a threadprivate variable is not an address constant.
+    // OpenMP [2.9.2, Restrictions, C/C++, p.10]
----------------
This citation shouldn't be here, because we're not checking this.


http://llvm-reviews.chandlerc.com/D356



More information about the cfe-commits mailing list