[PATCH] D109685: [flang][OpenMP] Add semantic check for threadprivate directive

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

In D109685#2997488 <https://reviews.llvm.org/D109685#2997488>, @kiranchandramohan wrote:

> LGTM. I have some Nit comments. Please fix it if possible.
>
> What about the other checks?

I am studying on them and they will come later. I prefer to separate semantic checks with different styles in multiple PRs to make review work easier, but I will keep similar semantic checks in one PR so that the code will not revised again and again.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1553-1561
+                        "A variable that is part of another variable (as an "
+                        "array or structure element) cannot appear in an %s "
+                        "directive"_err_en_US,
+                        ContextDirectiveAsFortran());
+                  } else {
+                    context_.Say(source,
+                        "A variable that is part of another variable (as an "
----------------
kiranchandramohan wrote:
> Nit: Would having a lambda be a better option? The lambda can take the clause or directive as an argument.
> 
Thanks for the advice. This kind of semantic check is only performed on allocate directive, threadprivate directive, declare target directive, private or shared clause. I replace the `comparison` check with `OmpDirectiveSet` currently and do not change the description of `PRIVATE or SHARED` into dynamic clause name, which is not necessary for current OpenMP 5.1 standard. I agree with you that lambda would be better if more clauses have this restriction.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1554
+                        "A variable that is part of another variable (as an "
+                        "array or structure element) cannot appear in an %s "
+                        "directive"_err_en_US,
----------------
kiranchandramohan wrote:
> Nit: an THREADPRIVATE -> a THREADPRIVATE
Fixed.


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

https://reviews.llvm.org/D109685