Re: [PATCH] [OPENMP] ‘linear’ clause codegen

[John McCall]

Mostly looks good.


================
Comment at: include/clang/AST/OpenMPClause.h:1388
@@ -1384,2 +1387,3 @@
         ColonLoc(SourceLocation()) {}
 
+  /// \brief Gets the list of initial values for linear variables.
----------------
Please add a comment describing the memory layout of the extra fields here.

================
Comment at: include/clang/AST/OpenMPClause.h:1421
@@ +1420,3 @@
+  /// \brief Sets the list of final update expressions to nullptr.
+  void clearFinals();
+
----------------
It looks like these 'clear' methods are called from one place; just inline them there.  If you'd rather not, please at least avoid calling them 'clear', which has different implications for C++ data structures.

================
Comment at: lib/AST/Stmt.cpp:1343
@@ -1313,3 +1342,3 @@
                                                   llvm::alignOf<Expr *>()) +
-                         sizeof(Expr *) * (NumVars + 1));
+                         4 * sizeof(Expr *) * (NumVars + 2));
   return new (Mem) OMPLinearClause(NumVars);
----------------
Please document the contributors to these magic numbers.

Also, if I'm understanding correctly, the calculation should be (4 * NumVars + 2) * sizeof(Expr*).

http://reviews.llvm.org/D8375

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/