Re: [PATCH] [OPENMP] ‘linear’ clause codegen
John McCall
rjmccall at gmail.com
Tue Mar 17 13:14:40 PDT 2015
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/
More information about the cfe-commits
mailing list