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

[Tim Keith via Phabricator via flang-commits]

tskeith added inline comments.


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:92
+        .Say(currentStatementSourcePosition_,
+            "%s to a construct name '%s' outside of %s construct is not allowed"_err_en_US,
+            stmt, branchingToName, upperCaseConstructName)
----------------
I suggest "%s to construct '%s' outside of..."


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:99
+      const parser::Name &stmtName, const parser::Name &constructName) const {
+    return stmtName.source == constructName.source;
+  }
----------------
I meant do the comparison in-line, and not define this function. Hiding it in a function makes it more obscure, IMO.


================
Comment at: flang/lib/Semantics/check-acc-structure.cpp:114
+      if (const parser::Name *
+          constructName{semantics::MaybeGetNodeName(construct)}) {
+        if (StmtNameMatchesConstructName(stmtName, *constructName)) {
----------------
Is `semantics::` necessary here?


================
Comment at: flang/lib/Semantics/check-do-forall.cpp:859
+  const parser::DoConstruct *doConstruct{
+      semantics::MaybeGetDoConstruct(construct)};
   return doConstruct && doConstruct->IsDoConcurrent();
----------------
`semantics::` ?


================
Comment at: flang/lib/Semantics/check-do-forall.cpp:918
     const ConstructNode &construct{*iter};
-    const parser::Name *constructName{MaybeGetNodeName(construct)};
+    const parser::Name *constructName{semantics::MaybeGetNodeName(construct)};
     if (StmtMatchesConstruct(stmtName, stmtType, constructName, construct)) {
----------------
`semantics::` ?


================
Comment at: flang/lib/Semantics/tools.cpp:1337
+                    },
+      construct);
+}
----------------
The call to `common::GetPtrFromOptional` could be moved outside of `std::visit` so that it doesn't have to be repeated in each lambda.

Even better might be to change the return type to `const <std::optional<parser::Name>> &`. Is there any reason to convert it to a pointer?


================
Comment at: flang/test/Semantics/acc-branch.f90:35
+  ! label is out of parallel construct, labelled block is not attached to an OpenACC parallel construct.
+  ! label is inside OpenACC construct.
+
----------------
You're still using the word "label" incorrectly here. There are no labels in this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87906