[flang-commits] [PATCH] D88965: [Flang][OpenMP] Rework parser changes for OpenMP atomic construct.

[Kiran Chandramohan via Phabricator via flang-commits]

kiranchandramohan added a comment.

A few minor/nit comments. Could you add a few unparse tests in the test/Parser directory?



================
Comment at: flang/include/flang/Parser/parse-tree.h:3598
   CharBlock source;
-  std::tuple<Verbatim, std::optional<Name>, std::optional<OmpHintExpr>> t;
+  std::tuple<Verbatim, std::optional<Name>, OmpClauseList> t;
 };
----------------
Why is it an OmpClauseList and not OmpClause? It is just a hint clause right? So should this be std::optional<OmpClause>?


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:162
+        construct<OmpClause>(parenthesized(Parser<OmpAllocateClause>{})) ||
+
     "COLLAPSE" >> construct<OmpClause>(construct<OmpClause::Collapse>(
----------------
Nit: empty line?


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:214
         construct<OmpClause>(parenthesized(Parser<OmpReductionClause>{})) ||
-    "ALLOCATE" >>
-        construct<OmpClause>(parenthesized(Parser<OmpAllocateClause>{})) ||
----------------
Thanks for moving this to the correct place.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:223
+    "RELEASE" >> construct<OmpClause>(construct<OmpClause::Release>()) ||
+
     "SAFELEN" >> construct<OmpClause>(construct<OmpClause::Safelen>(
----------------
Nit: Empty Line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88965