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

[Andrzej Warzynski via Phabricator via cfe-commits]

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