[flang-commits] [PATCH] D93051: [Flang][openmp] Add semantic checks for OpenMP critical construct.

[Valentin Clement via Phabricator via flang-commits]

clementval added inline comments.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1000
+  PushContext(beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  PushContext(endCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  if (const auto &criticalName{
----------------
Does it really make sense to push two contexts? If yes, you might want to have a visitor function for `parser::OmpCriticalDirective` and `parser::OmpEndCriticalDirective` where you push their respective context. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1074
+    } else {
+      DIE("OpenMP Name resolution failed");
+    }
----------------
Do you want the program to abort or would it be better to have a return value to this function and generate appropriate user facing error message? 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1076
+    }
+  }
+
----------------
Are those tabs? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93051