[PATCH] D93205: [Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs for single directive

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:140
 
+  void SetLoopIv(Symbol *symbol) { GetContext().loopIV = symbol; }
   // back() is the top of the stack
----------------
Maybe add a blank line here to keep with the style of this file? 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:96
   }
+  CheckLoopIv(x);
+}
----------------
Doesn't really check smth but set a state in the context. Should it be renamed ? 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:106
+void OmpStructureChecker::CheckLoopIv(const parser::OpenMPLoopConstruct &x) {
+
+  if (const auto &loopConstruct{
----------------
Remove blank line. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:153
+  case llvm::omp::OMPD_single:
+    for (const auto &dirContext : dirContext_) {
+      if (dirContext.directive == llvm::omp::Directive::OMPD_do) {
----------------
`dirContext_` act as a stack so do you really intent to go from the bottom of the stack to the top? If so no problem. Just asking. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:494
+  CheckAllowed(llvm::omp::Clause::OMPC_firstprivate);
+
+  for (const auto &ompObject : x.v.v) {
----------------
Extra blank line. 


================
Comment at: flang/lib/Semantics/check-omp-structure.h:198
       const parser::OmpObjectList &, std::vector<const Symbol *> &);
+
+  const parser::Name GetLoopIndex(const parser::DoConstruct *x);
----------------
Remove blank line. Style is not really consistent across file but let's try to keep with the same style in a single file. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93205