[PATCH] D87906: [Flang][OpenACC] Fix for branching out issue in OpenACC parallel construct.

[Tim Keith via Phabricator via llvm-commits]

tskeith requested changes to this revision.
tskeith added inline comments.


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:56
+        std::get<parser::Statement<parser::NonLabelDoStmt>>(doConstruct.t)};
+    if (const auto *currentLabel{MaybeGetStmtName(doStmt.statement)}) {
+      const Symbol &sym{*currentLabel->symbol};
----------------
Why are you converting to a pointer?

I suggest:
`if (const auto &name{std::get<std::optional<parser::Name>>(doStmt.statement.t)}) {`


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:89
+  void Post(const parser::ExitStmt &exitStmt) {
+    if (const auto *exitLabel{common::GetPtrFromOptional(exitStmt.v)}) {
+      const Symbol &sym{*exitLabel->symbol};
----------------
Why convert to a pointer here?


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:105
         .Say(currentStatementSourcePosition_,
             "%s statement is not allowed in a %s construct"_err_en_US, stmt,
             parser::ToUpperCaseLetters(
----------------
This message needs to be fixed to match the change. RETURN and EXIT are now allowed in some cases so it is not accurate.


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:175
+  // Works on struct Name and not the Label(R611).
+  ConstructSymbolSet doConstructLabelSet;
+  parser::Walk(block, doConstructLabelSet);
----------------
`doConstructNameSet` would be a much better name. Then you don't need comments explaining why you chose a confusing name.


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:176
+  ConstructSymbolSet doConstructLabelSet;
+  parser::Walk(block, doConstructLabelSet);
+  NoBranchingEnforce noBranchingEnforce{
----------------
Can't you do this as part of NoBranchingEnforce? There is a lot of overhead to having another parse tree visitor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87906