[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`
Tim Keith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 1 08:12:09 PST 2021
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
More information about the cfe-commits
mailing list