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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
