[PATCH] D92732: [Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

One question about blocks and a few nits.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:109
+      it = labelNamesandLevels_.find(cyclestmt.v->source.ToString());
+      err = (it != labelNamesandLevels_.end() && it->second > 0) ? true : false;
+    }
----------------
Nit this can just be

```
err = (it != labelNamesandLevels_.end() && it->second > 0);
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:125
+  SemanticsContext &context_;
+  const parser::CharBlock *cyclesource_;
+  std::int64_t cycleLevel_;
----------------
Nit: cyclesource_ -> cycleSource_


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:246-248
+      const auto it{block.begin()};
+      loop = it != block.end() ? parser::Unwrap<parser::DoConstruct>(*it)
+                               : nullptr;
----------------
Is something missing here?
If "it" is block.begin() in the previous patch, how can it ever be block.end in this line unless the block is empty?

If nothing is missing, add a comment.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:277
+    orderedCollapseLevel = orderedLevel;
+  } else if (collapseLevel > orderedLevel) {
+    orderedCollapseLevel = collapseLevel;
----------------
Nit: this condition is always true right? if so it can be reduced to an else statement (from else if).


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:217
+        context_.Say(doStmt.source,
+            "The do loop cannot be a DO WHILE with do directive."_err_en_US);
+      }
----------------
kiranchandramohan wrote:
> Can the "do" be capitalized for the loop and the directive?
> 
Nit: Capitalize for the directive as well. 
"The DO loop cannot be a DO WHILE with DO directive."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732