awarzynski added a comment.
@arnamoy10 Thank you for addressing my comments!
As for testing that `-J/-module-dir` are taken into account when specifying the
output directory for modules, could try adding the following:
! RUN: mkdir -p %t/dir-f18 && %f18 -fparse-only -I tools/flang/include/flang
-module %t/dir-f18 %s 2>&1
! RUN: ls %t/dir/testmodule.mod && not ls %t/testmodule.mod
! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir
%t/dir-flang-new %s 2>&1
! RUN: ls %t/dir/testmodule.mod && not ls %t/testmodule.mod
module testmodule
type::t2
end type
end
It's possible that `flang-new` doesn't support writing modules yet, but IMHO
this is the right moment to try and understand what might be missing. Thank you!
================
Comment at: clang/include/clang/Driver/Options.td:1006-1007
MarshallingInfoString<DependencyOutputOpts<"ModuleDependencyOutputDir">>;
+def module_dir : Separate<["-"], "module-dir">,
+ Flags<[FlangOption,FC1Option]>, HelpText<"Add to the list of directories to
be searched by an USE statement">;
def dsym_dir : JoinedOrSeparate<["-"], "dsym-dir">,
----------------
awarzynski wrote:
> As we are trying to follow `gfortran`, I suggest that we copy the help
> message from there:
>
> ```
> $ gfortran --help=separate | grep '\-J'
> -J<directory> Put MODULE files in 'directory'
> ```
> Also, we can add the long version (via `DocBrief` field) from here
> https://gcc.gnu.org/onlinedocs/gfortran/Directory-Options.html:
> ```
> This option specifies where to put .mod files for compiled modules. It is
> also added to the list of directories to searched by an USE statement.
>
> The default is the current directory.
> ```
>
> I appreciate that this patch only implements the 2nd part of what the option
> is intended to offer (i.e. updates the search patch for module files). But I
> think that it's worthwhile to make the intent behind this option clear from
> the very beginning. We can use the commit message to document the current
> limitations.
>
> Also, please keep in mind that this help message is going to be re-used by
> `-J`, which belongs to `gfortran_Group`. So the description needs to be valid
> for both.
No indentation in `DocBrief`, see this [[
https://github.com/llvm/llvm-project/blob/21bfd068b32ece1c6fbc912208e7cd1782a8c3fc/clang/include/clang/Driver/Options.td#L680-L688
| example ]]
Also, `Put MODULE files in 'directory'` -> `Put MODULE files in <dir>` (the
option is displayed as `-module-dir <dir>`).
================
Comment at: clang/include/clang/Driver/Options.td:4124
def A_DASH : Joined<["-"], "A-">, Group<gfortran_Group>;
-def J : JoinedOrSeparate<["-"], "J">, Flags<[RenderJoined]>,
Group<gfortran_Group>;
def cpp : Flag<["-"], "cpp">, Group<gfortran_Group>;
----------------
There's no need to move this, is there? I feel that it's better to keep all
`gfortran` options together.
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
/// {
+ Fortran::semantics::SemanticsContext &semaChecking() const { return
*semantics_; }
----------------
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.
================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+ semantics_(new Fortran::semantics::SemanticsContext(*(new
Fortran::common::IntrinsicTypeDefaultKinds()),*(new
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
----------------
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!
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:302
+
+void CompilerInvocation::setSemanticsOpts(Fortran::semantics::SemanticsContext
&semaCtxt) {
+ auto &fortranOptions = fortranOpts();
----------------
The argument is not needed here, is it? You could just write:
```
auto &semaCtx = semanticsContext()
```
Or am I missing something?
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:99
- // Prepare semantics
- Fortran::semantics::SemanticsContext semanticsContext{
- defaultKinds, features, ci.allCookedSources()};
+ // Prepare semantics
Fortran::semantics::Semantics semantics{
----------------
[nit] Unrelated change
================
Comment at: flang/test/Flang-Driver/include-module.f90:15
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs %s 2>&1 | FileCheck
%s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s
2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs/module-dir -module-dir
%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
----------------
Why `--check-prefix=SINGLEINCLUDE` here and below? Both directories are
included, so there should be no errors.
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
