[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`

[Tim Keith via Phabricator via cfe-commits]

tskeith added inline comments.


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
   /// {
+  Fortran::semantics::SemanticsContext &semaChecking() const { return *semantics_; }
 
----------------
awarzynski wrote:
> tskeith wrote:
> > `semanticsContext` would be a better name for this function.
> We should follow Flang's [[ https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#naming | coding style ]] here:
> ```
> Accessor member functions are named with the non-public data member's name, less the trailing underscore. 
> ```
> i.e. `semantics()` rather than `semanticsContext()`. If we were to diverge from that, then I suggest that we follow the style prevalent in LLVM/Clang, see e.g. [[ https://github.com/llvm/llvm-project/blob/21bfd068b32ece1c6fbc912208e7cd1782a8c3fc/clang/include/clang/Frontend/CompilerInstance.h#L503-L506 | getSema ]].
> 
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_
> Makes sense to me. 
> 
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_

Right.



================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+      semantics_(new Fortran::semantics::SemanticsContext(*(new  Fortran::common::IntrinsicTypeDefaultKinds()),*(new common::LanguageFeatureControl()),
+                 *allCookedSources_)) {
 
----------------
awarzynski wrote:
> tskeith wrote:
> > Why is `semantics_` a `shared_ptr` rather than a simple data member of  type `SemanticsContext`? It's owned by only `CompilerInstance`, not shared.
> > The same question probably applies to the other fields too.
> @tskeith You raise two important points here:
> 
> **Why shared_ptr if the resource is not shared?** 
> From what I can see, at this point the `SemanticsContext` is not shared and we can safely use `unique_ptr` instead.
> 
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all classes/structs that are required for creating a _compiler instance_. It's kept lightweight and written in a way that makes it easy to _inject_ custom instances of these classes. This approach is expected to be helpful when creating new frontend actions (I expect that there will be a lot) and when compiling projects with many source files.
> 
> @tskeith, I intend to document the design of the new driver soon and suggest that that's when we re-open the discussion on the design of `CompilerInstance`.
> 
> IMO this change is consistent with the current design and I think that we should accept it as is.
> 
> **Small suggestion**
> @arnamoy10 , I think that you can safely add `IntrinsicTypeDefaultKinds` and `LanguageFeatureControl` members too. We will need them shortly and this way this constructor becomes much cleaner. I'm fine either way!
> 
> 
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all classes/structs that are required for creating a _compiler instance_.

As it stands it does own the instance of `SemanticsContext` etc. No one else does.

> @tskeith, I intend to document the design of the new driver soon and suggest that that's when we re-open the discussion on the design of `CompilerInstance`.

OK



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

https://reviews.llvm.org/D95448