[flang-commits] [PATCH] D94087: [flang][openmp]At most one threads, simd and depend clause can appear on OpenMP ORDERED construct.

[sameeran joshi via Phabricator via flang-commits]

sameeranjoshi marked an inline comment as done.
sameeranjoshi added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:138
 
-void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
+void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &x) {
+  const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
----------------
kiranchandramohan wrote:
> Why is this handled on Leave and not Enter like others?
With the current approach (traversing an `OmpClauseList`) it could have worked inside `Enter`.
But here's are a few reasons not to use 

* I prefer doing checks inside `Leave`, so that the checks remain at a single location and `Enter` does the context pushing and other stuff.
* I had initially plan to use `FindClause` as that saves us from writing more code and code looks more readable, unfortunately that did not work in the first run as `ResetPartialContext` clears the members in `DirectiveContext` which has a reason because `OmpBeginBlockDirective` & `OmpEndBlockDirective` have different set of clauses based on the context.
To resolve this will add `Leave` for `OmpBeginBlockDirective`.
```
  void Enter(const parser::OpenMPBlockConstruct &);
  void Leave(const parser::OpenMPBlockConstruct &);
  void Leave(const parser::OmpBeginBlockDirective &);
  void Enter(const parser::OmpEndBlockDirective &);
```
* `FindClause` doesn't work inside `Enter` as the list is empty and it's populated only when we visit each clause.
   `grep FindClause` and see it's not found inside any `Enter` function.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:239
 void OmpStructureChecker::Leave(
-    const parser::OpenMPSimpleStandaloneConstruct &) {
+    const parser::OpenMPSimpleStandaloneConstruct &x) {
+  const auto &beginStandaloneDir{
----------------
kiranchandramohan wrote:
> Same question here.
The case of `OpenMPSimpleStandaloneConstruct` is different as there is no `ResetPartialContext` nor any end constructs nor directives for it.
So doing the check here has below reason:

* `FindClause` will only work inside `Leave`.
* A style approach.

With the next diff I have plan to use `FindClause`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94087