[PATCH] D83998: [flang][openacc] Basic name resolution infrastructure for OpenACC construct

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 7 inline comments as done.
clementval added a comment.

@klausler Thanks for the feedback. Will wait on @tskeith inputs before going forward.



================
Comment at: flang/lib/Semantics/resolve-names.cpp:1085
 
+template <typename T> class DirectiveAttributeVisitor {
+public:
----------------
klausler wrote:
> clementval wrote:
> > klausler wrote:
> > > clementval wrote:
> > > > klausler wrote:
> > > > > clementval wrote:
> > > > > > clementval wrote:
> > > > > > > klausler wrote:
> > > > > > > > resolve-names.cpp is already a large source file.  Does the directive name resolution code have to be in there in order to use things that are local to that file?  If not, maybe all the directive name resolution code could reside elsewhere.
> > > > > > > I think we can move that into a separate file. I'll update the patch. Thanks for the suggestion.
> > > > > > @klausler I would have to move some class declaration from `resolve-name.cpp` to `resolve-name.h`. If this is fine I will split the files. 
> > > > > Which classes?
> > > > > 
> > > > > Can they go into resolve-names-utils.{h,cpp} instead?
> > > > In fact most of the class declarations would have to be moved to the `resolve-name.h` file in order to be able to split the file and have its own for directive resolution.
> > > That may be the case for `AccVisitor`, but can `AccAttributeVisitor` be extracted into its own header and C++ source file?
> > After some work on this, this would need to move all the declarations in to a header file (`resolve-name.h`) because the `Acc/OmpVisitor` and `Acc/OmpAttributeVisitor` use classes declared inside `resolve-names.cpp` and those classes derived from other internal classes as well. 
> > 
> > It would be much easier to keep those declarations together. One solution to reduce the size of the file might be to extract the declarations in the `.h` file and keep only the implementation in the `.cpp` file but still together. Introducing new files for the directive part makes it harder since they are interdependent. 
> > 
> > What do you think is the best to move forward @klausler? 
> Please don't embark on a wholesale restructuring of resolve-names.cpp without consulting with @tskeith.  If there isn't an obvious way to extract the name resolution for directives, just leave it in there.
The problem is that `AccAttributeVisitor`, `OmpAttributeVisitor` and `DirectiveAttributeVisitor` are using `ResolveNamesVisitor` that is not declared in a `.h` file but in `resolve-names.cpp`. This `class` needs then lots of other classes declared in `resolve-names.cpp`. So there is no easy way to extract them without a refactoring. 


================
Comment at: flang/lib/Semantics/resolve-names.cpp:1085
 
+template <typename T> class DirectiveAttributeVisitor {
+public:
----------------
clementval wrote:
> klausler wrote:
> > clementval wrote:
> > > klausler wrote:
> > > > clementval wrote:
> > > > > klausler wrote:
> > > > > > clementval wrote:
> > > > > > > clementval wrote:
> > > > > > > > klausler wrote:
> > > > > > > > > resolve-names.cpp is already a large source file.  Does the directive name resolution code have to be in there in order to use things that are local to that file?  If not, maybe all the directive name resolution code could reside elsewhere.
> > > > > > > > I think we can move that into a separate file. I'll update the patch. Thanks for the suggestion.
> > > > > > > @klausler I would have to move some class declaration from `resolve-name.cpp` to `resolve-name.h`. If this is fine I will split the files. 
> > > > > > Which classes?
> > > > > > 
> > > > > > Can they go into resolve-names-utils.{h,cpp} instead?
> > > > > In fact most of the class declarations would have to be moved to the `resolve-name.h` file in order to be able to split the file and have its own for directive resolution.
> > > > That may be the case for `AccVisitor`, but can `AccAttributeVisitor` be extracted into its own header and C++ source file?
> > > After some work on this, this would need to move all the declarations in to a header file (`resolve-name.h`) because the `Acc/OmpVisitor` and `Acc/OmpAttributeVisitor` use classes declared inside `resolve-names.cpp` and those classes derived from other internal classes as well. 
> > > 
> > > It would be much easier to keep those declarations together. One solution to reduce the size of the file might be to extract the declarations in the `.h` file and keep only the implementation in the `.cpp` file but still together. Introducing new files for the directive part makes it harder since they are interdependent. 
> > > 
> > > What do you think is the best to move forward @klausler? 
> > Please don't embark on a wholesale restructuring of resolve-names.cpp without consulting with @tskeith.  If there isn't an obvious way to extract the name resolution for directives, just leave it in there.
> The problem is that `AccAttributeVisitor`, `OmpAttributeVisitor` and `DirectiveAttributeVisitor` are using `ResolveNamesVisitor` that is not declared in a `.h` file but in `resolve-names.cpp`. This `class` needs then lots of other classes declared in `resolve-names.cpp`. So there is no easy way to extract them without a refactoring. 
Sure will wait on his inputs. 


================
Comment at: flang/lib/Semantics/resolve-names.cpp:1085
 
+template <typename T> class DirectiveAttributeVisitor {
+public:
----------------
clementval wrote:
> clementval wrote:
> > klausler wrote:
> > > clementval wrote:
> > > > klausler wrote:
> > > > > clementval wrote:
> > > > > > klausler wrote:
> > > > > > > clementval wrote:
> > > > > > > > clementval wrote:
> > > > > > > > > klausler wrote:
> > > > > > > > > > resolve-names.cpp is already a large source file.  Does the directive name resolution code have to be in there in order to use things that are local to that file?  If not, maybe all the directive name resolution code could reside elsewhere.
> > > > > > > > > I think we can move that into a separate file. I'll update the patch. Thanks for the suggestion.
> > > > > > > > @klausler I would have to move some class declaration from `resolve-name.cpp` to `resolve-name.h`. If this is fine I will split the files. 
> > > > > > > Which classes?
> > > > > > > 
> > > > > > > Can they go into resolve-names-utils.{h,cpp} instead?
> > > > > > In fact most of the class declarations would have to be moved to the `resolve-name.h` file in order to be able to split the file and have its own for directive resolution.
> > > > > That may be the case for `AccVisitor`, but can `AccAttributeVisitor` be extracted into its own header and C++ source file?
> > > > After some work on this, this would need to move all the declarations in to a header file (`resolve-name.h`) because the `Acc/OmpVisitor` and `Acc/OmpAttributeVisitor` use classes declared inside `resolve-names.cpp` and those classes derived from other internal classes as well. 
> > > > 
> > > > It would be much easier to keep those declarations together. One solution to reduce the size of the file might be to extract the declarations in the `.h` file and keep only the implementation in the `.cpp` file but still together. Introducing new files for the directive part makes it harder since they are interdependent. 
> > > > 
> > > > What do you think is the best to move forward @klausler? 
> > > Please don't embark on a wholesale restructuring of resolve-names.cpp without consulting with @tskeith.  If there isn't an obvious way to extract the name resolution for directives, just leave it in there.
> > The problem is that `AccAttributeVisitor`, `OmpAttributeVisitor` and `DirectiveAttributeVisitor` are using `ResolveNamesVisitor` that is not declared in a `.h` file but in `resolve-names.cpp`. This `class` needs then lots of other classes declared in `resolve-names.cpp`. So there is no easy way to extract them without a refactoring. 
> Sure will wait on his inputs. 
Basically almost all the classes declared in `resolve-names.cpp` because one inherits from another or uses other classes and we end up moving the whole things. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83998