[PATCH] D76211: OpenMP Metadirective with user defined condition

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:863
+/// clause with user defined condition.
+class OMPWhenClause : public OMPClause {
+  friend class OMPClauseReader;
----------------
`final`


================
Comment at: clang/include/clang/AST/OpenMPClause.h:866
+
+  Expr *expr;
+  OpenMPDirectiveKind DKind;
----------------
Reformat the patch


================
Comment at: clang/include/clang/AST/OpenMPClause.h:868
+  OpenMPDirectiveKind DKind;
+  SmallVector<OMPClause *, 5> Clauses;
+
----------------
Bad idea. If you need to store other clauses in this clause, use tail allocation.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:906
+  /// Returns the clauses associated with the directive variants
+  SmallVector<OMPClause *, 5> getClauses() { return Clauses; }
+
----------------
Must return `ArrayRef`


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:348
 
+class OMPMetaDirective : public OMPExecutableDirective {
+  friend class ASTStmtReader;
----------------
1. `final`
2. Comments?


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:350
+  friend class ASTStmtReader;
+  Stmt* IfStmt;
+
----------------
This must be a part of the tail-allocated data members, otherwise you won't be able to correctly handle list of children nodes


================
Comment at: clang/lib/Serialization/ASTReader.cpp:11902
+  C->setExpr(Record.readSubExpr());
+  C->setLParenLoc(Record.readSourceLocation());
+}
----------------
Serialization for inner clauses


================
Comment at: clang/test/OpenMP/metadirective_default.c:1-13
+#include <stdio.h>
+#include <omp.h>
+
+int main()
+{
+  int N = 5;
+#pragma omp metadirective when(user={condition(N>10)}: parallel) \
----------------
This is not a test, check other files in this directory


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76211