[PATCH] D98558: [OPENMP51]Initial support for the interop directive

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:7423-7424
+  /// preference-list.
+  /// \param IsTarget Uses the 'target' interop-type.
+  /// \param IsTargetSync Uses the 'targetsync' interop-type.
+  /// \param StartLoc Starting location of the clause.
----------------
mikerice wrote:
> ABataev wrote:
> > Do we really need these params? I assume they are already part of `Exprs` though in `Expr *` form.
> The 'target' and 'targetsync' interop-types are not parsed as Exprs.  Are you suggesting we should create Exprs for these?
Oh, removed the wrong comment :( 
I suggest splitting `Exprs` parameter and do not include interop variable there, pass this variable in a separate param, but still store it as part of `Expr*` list


================
Comment at: clang/include/clang/AST/OpenMPClause.h:7477
+    auto Children = const_cast<OMPInitClause *>(this)->children();
+    return const_child_range(++Children.begin(), Children.end());
+  }
----------------
Beter to use `std::next(Children.begin())` or `Children.begin() + 1`


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1781
+  bool First = true;
+  for (const auto *E : Node->prefs()) {
+    if (First)
----------------
`auto`->`Expr`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:14631
+  bool IsTargetSync = false;
+  for (const auto *C : Clauses) {
+    if (IsTargetSync)
----------------
`auto`->`OMPClause`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:14648
+  llvm::SmallSetVector<const VarDecl *, 4> InteropVars;
+  for (const auto *C : Clauses) {
+    auto ClauseKind = C->getClauseKind();
----------------
`auto`->`OMPClause`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:14649
+  for (const auto *C : Clauses) {
+    auto ClauseKind = C->getClauseKind();
+    const DeclRefExpr *DRE;
----------------
`auto`->`OMPClauseKind`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:14653-14663
+    switch (ClauseKind) {
+    case OMPC_init: {
+      const auto *IC = cast<OMPInitClause>(C);
+      VarLoc = IC->getVarLoc();
+      DRE = dyn_cast_or_null<DeclRefExpr>(IC->getInteropVar());
+      break;
+    }
----------------
Better to use `if (ClauseKind == OMPC_init) ` here


================
Comment at: clang/lib/Sema/TreeTransform.h:9305
+  Exprs.reserve(C->varlist_size());
+  for (auto *E : C->varlists()) {
+    ExprResult ER = getDerived().TransformExpr(cast<Expr>(E));
----------------
`auto`->`Expr`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12140
+  Vars.reserve(NumVars);
+  for (unsigned i = 0; i != NumVars; ++i)
+    Vars.push_back(Record.readSubExpr());
----------------
`i`->`I`


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:6220
+  Record.push_back(C->varlist_size());
+  for (auto *VE : C->varlists()) {
+    Record.AddStmt(VE);
----------------
1. `auto`->`Expr`
2> No need for braces


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

https://reviews.llvm.org/D98558