[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:369
   void ResolveOmpName(const parser::Name &, Symbol::Flag);
-  Symbol *ResolveName(const parser::Name *);
+  Symbol *ResolveName(const parser::Name *, Symbol::Flag);
   Symbol *DeclareOrMarkOtherAccessEntity(const parser::Name &, Symbol::Flag);
----------------
sameeranjoshi wrote:
> clementval wrote:
> > What's the big difference with `ResolveOmpName`. Merging the two functions would be better. 
> That's a good catch.
> If you see `ResolveName` was implemented before this patch.
> This patch highlighted the need to extend the name resolution logic, which was just extended inside `ResolveName` and no other code was affected(except that we need a one time signature change with this patch).
> As discussed, there is no way I could find to reuse code from `resolve-names.cpp` inside this file, hence we had a short/quick way to implement `ResolveName` inside this file.
> Being said that I still see with some new construct the `ResolveName` inside this file might need some change.
> Hence I extracted into another function.
> Which makes only changes to the above function and not much changes in other code.
> 
> If there's still something you see of no use to have separate functions I can refactor them and collapse them into 1.
> Thank you.
Would be better to have a single function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93051