[PATCH] D116764: [clang][OpenMP5.1] Initial parsing/sema for 'indirect' clause

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

Thanks Alexey for the code review.



================
Comment at: clang/include/clang/Sema/Sema.h:10338
+    Expr *IndirectExpr = nullptr;
+    bool IsIndirect = false;
+
----------------
ABataev wrote:
> Can you use `Optional<Expr *>` instead of `Expr *` and `bool`?
Good to know.  Changed.  Thank you so much!!!!


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1862
+      if (IsIndirectClause) {
+        if (!ParseOpenMPIndirectClause(DTCI, /*ParseOlyn*/false))
+          break;
----------------
ABataev wrote:
> `/*ParseOnly=*/`
Sorry changed.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3427
+
+  DTCI.IndirectExpr = Val.get();
+  if (Val.isInvalid())
----------------
ABataev wrote:
> I assume it shall be set only if `Actions.ActOnOpenMPIndirectClause` is successful.
The IdirectExpr is user Expression and use for AST dump.  Indirect is one which is set after ActOpenMPIndirectClause.

BTW the indirect[(invoked-by-fptr)]
where invoked-by-fptr is a constant boolean expression that evaluates to true or false at compile time.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15732
 
+bool Sema::ActOnOpenMPIndirectClause(Expr *Condition,
+                                     SourceLocation StartLoc,
----------------
ABataev wrote:
> I assume it shall return `ExprResult`
Add check inside ParseOpenMPIndirectClause instead.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15747-15748
+      return false;
+    if (Result.getBoolValue() == true)
+      return true;
+  }
----------------
ABataev wrote:
> Is this correct? Does `Sema::ActOnOpenMPIndirectClause` is supposed to be failed if `Result` is `false`?
This is only set for IsIndirect for DTCI which is  for Indirect(true).   The original expression is passed in IndirectExpr.  
Since you suggest using optional<Expr *>,  Rewrite it move the code inside ParseOpenMPIndirectClause


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116764