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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1006-1007
   MarshallingInfoString>;
+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:
> 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   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 ` (the 
> option is displayed as `-module-dir `).
Is the clang file change needed?

`clang -help` now has `  -module-dirPut MODULE files in ` 
while I think the option only makes sense for flang.


Repository:
  rG LLVM Github Monorepo

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG985a42fdf8ae: [flang][driver] Add support for 
`-J/-module-dir` (authored by arnamoy10, committed by awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D95448?vs=320617=321448#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/include-module.f90
  flang/test/Flang-Driver/write-module.f90

Index: flang/test/Flang-Driver/write-module.f90
===
--- /dev/null
+++ flang/test/Flang-Driver/write-module.f90
@@ -0,0 +1,10 @@
+! 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-f18/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-flang-new/testmodule.mod && not ls %t/testmodule.mod
+
+module testmodule
+  type::t2
+  end type
+end
Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -7,12 +7,26 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -I %S/Inputs -J %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -I %S/Inputs -module-dir %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! 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=DOUBLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -J %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -module-dir %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
@@ -22,6 +36,11 @@
 ! SINGLEINCLUDE-NOT:error: Derived type 't1' not found
 ! SINGLEINCLUDE:error: Derived type 't2' not found
 
+!-
+! EXPECTED OUTPUT FOR MISSING MODULE FILE
+!-
+! DOUBLEINCLUDE:error: Only one '-module-dir/-J' option allowed
+
 !---
 ! EXPECTED OUTPUT FOR ALL MODULES FOUND
 !---
Index: flang/test/Flang-Driver/driver-help.f90
===
--- flang/test/Flang-Driver/driver-help.f90
+++ flang/test/Flang-Driver/driver-help.f90
@@ -30,6 +30,7 @@
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! 

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 added a comment.

Sounds good to me, thanks for the help.  In the meantime I will work on the 
`fdefault-*` family of flags, which will be dependent on this patch I think.




Comment at: clang/include/clang/Driver/Options.td:4124
 def A_DASH : Joined<["-"], "A-">, Group;
-def J : JoinedOrSeparate<["-"], "J">, Flags<[RenderJoined]>, 
Group;
 def cpp : Flag<["-"], "cpp">, Group;

awarzynski wrote:
> There's no need to move this, is there? I feel that it's better to keep all 
> `gfortran` options together.
This needs to be moved, as we are using Aliases.  The way Aliases work is (in 
this order) (1) you create the base option (that 



Comment at: clang/include/clang/Driver/Options.td:1018
+  an USE statement.  The default is the current 
directory.}]>,Group;
+def module_dir : Separate<["-"], "module-dir">, 
Flags<[FlangOption,FC1Option]>, MetaVarName<"">, Alias;
 def dsym_dir : JoinedOrSeparate<["-"], "dsym-dir">,

tskeith wrote:
> It would be better to make `-module-dir` the main option and `-J` the alias. 
> `-J` is only there for gfortran compatibility, not because it is a good name 
> for the option.
Sure, we can do that.  I just chose -J to be default as it is easier to type 
for the user :)



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.
No idea about this design decision.  I have not found any other DS that is 
"sharing" these pointers.  I can take them out.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:302
+
+void CompilerInvocation::setSemanticsOpts(Fortran::semantics::SemanticsContext 
) {
+  auto  = fortranOpts();

awarzynski wrote:
> The argument is not needed here, is it? You could just write:
> ```
> auto  = semanticsContext()
> ```
> Or am I missing something?
`semanticsContext_` belongs to `CompilerInstance`, not `CompilerInvocation`, so 
unfortunately that is not possible.



Comment at: flang/test/Flang-Driver/include-module.f90:11
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s 
--check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s  
2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s 
--check-prefix=SINGLEINCLUDE

awarzynski wrote:
> What about:
> *  `-J %S/Inputs -J %S/Inputs/module-dir` (i.e. `-J` + `-J`)
> * `-J %S/Inputs -module-dir %S/Inputs/module-dir` (i.e. `-J` + `-module-dir`)
> * `-module-dir %S/Inputs -J%S/Inputs/module-dir` (i.e. `-module-dir` + `-J`)
> * `-module-dir %S/Inputs -I%S/Inputs/module-dir` (.e. `-module-dir` + `-I`)
> 
> I appreciate that this is a bit tedious, but IMO it is worthwhile to add a 
> few more cases here to avoid any unpleasant breakage in the future. Also - 
> what should the relation between `-I` and `-J` be here? As in, which ought to 
> have higher priority? 
Done


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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

LGTM!

Thank you for addressing my comments, (`DELETEME` can be fixed when pushing 
upstream)! From what I can see you've also addressed all of Tim's comments, but 
could you wait a day or two before merging this? Just in case I missed 
something, or Tim or somebody else has further comments.




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:191-192
+  
+  //for (const auto *currentArg : 
args.filtered(clang::driver::options::OPT_module_dir))  
+  //   moduleDir = currentArg->getValue();
+  auto moduleDirList = 
args.getAllArgValues(clang::driver::options::OPT_module_dir);

DELETEME



Comment at: flang/lib/Frontend/FrontendActions.cpp:81-82
   // TODO: These should be specifiable by users. For now just use the defaults.
-  common::LanguageFeatureControl features;
-  Fortran::common::IntrinsicTypeDefaultKinds defaultKinds;
+  // common::LanguageFeatureControl features;
+  // Fortran::common::IntrinsicTypeDefaultKinds defaultKinds;
 

DELETEME


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

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 updated this revision to Diff 320617.
arnamoy10 added a comment.

A few more comments addressed and a new test case added for write-module check.


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

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/include-module.f90
  flang/test/Flang-Driver/write-module.f90

Index: flang/test/Flang-Driver/write-module.f90
===
--- /dev/null
+++ flang/test/Flang-Driver/write-module.f90
@@ -0,0 +1,10 @@
+! 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-f18/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-flang-new/testmodule.mod && not ls %t/testmodule.mod
+
+module testmodule
+  type::t2
+  end type
+end
Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -7,12 +7,26 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -I %S/Inputs -J %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -I %S/Inputs -module-dir %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! 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=DOUBLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -J %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -module-dir %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=DOUBLEINCLUDE
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
@@ -22,6 +36,11 @@
 ! SINGLEINCLUDE-NOT:error: Derived type 't1' not found
 ! SINGLEINCLUDE:error: Derived type 't2' not found
 
+!-
+! EXPECTED OUTPUT FOR MISSING MODULE FILE
+!-
+! DOUBLEINCLUDE:error: Only one '-module-dir/-J' option allowed
+
 !---
 ! EXPECTED OUTPUT FOR ALL MODULES FOUND
 !---
Index: flang/test/Flang-Driver/driver-help.f90
===
--- flang/test/Flang-Driver/driver-help.f90
+++ flang/test/Flang-Driver/driver-help.f90
@@ -26,6 +26,7 @@
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! HELP-NEXT: -help  Display available options
 ! HELP-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-NEXT: 

[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 () 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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



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

awarzynski wrote:
> Why `--check-prefix=SINGLEINCLUDE` here and below? Both directories are 
> included, so there should be no errors.
Apologies, I was wrong with regard to how `-J/-module-dir` should work:

```
$ gfortran -J test-dir/ -J test-dir/ test.f
f951: Fatal Error: gfortran: Only one ‘-J’ option allowed
compilation terminated.
```

`gfortran` behavior makes a lot of sense to me and I suggest that we replicate 
that. This means that we should issue a diagnostic when `-J/-module-dir` is 
used twice.


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

[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>;
+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   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 ` (the 
option is displayed as `-module-dir `).



Comment at: clang/include/clang/Driver/Options.td:4124
 def A_DASH : Joined<["-"], "A-">, Group;
-def J : JoinedOrSeparate<["-"], "J">, Flags<[RenderJoined]>, 
Group;
 def cpp : Flag<["-"], "cpp">, 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 () 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 

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 updated this revision to Diff 320306.
arnamoy10 added a comment.

Addressing reviewers' comments, adding `-module-dir` as the default option 
instead of `-J`


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

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/include-module.f90

Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -8,12 +8,28 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! 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
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
Index: flang/test/Flang-Driver/driver-help.f90
===
--- flang/test/Flang-Driver/driver-help.f90
+++ flang/test/Flang-Driver/driver-help.f90
@@ -26,6 +26,7 @@
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! HELP-NEXT: -help  Display available options
 ! HELP-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-NEXT: -module-dirPut MODULE files in 'directory'
 ! HELP-NEXT: -o   Write output to 
 ! HELP-NEXT: -U  Undefine macro 
 ! HELP-NEXT: --version  Print version information
@@ -41,6 +42,7 @@
 ! HELP-FC1-NEXT: -E Only run the preprocessor
 ! HELP-FC1-NEXT: -help  Display available options
 ! HELP-FC1-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-FC1-NEXT: -module-dirPut MODULE files in 'directory'
 ! HELP-FC1-NEXT: -o   Write output to 
 ! HELP-FC1-NEXT: -U  Undefine macro 
 ! HELP-FC1-NEXT: --version  Print version information
Index: flang/test/Flang-Driver/driver-help-hidden.f90

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

[Tim Keith via Phabricator via cfe-commits]

tskeith added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1018
+  an USE statement.  The default is the current 
directory.}]>,Group;
+def module_dir : Separate<["-"], "module-dir">, 
Flags<[FlangOption,FC1Option]>, MetaVarName<"">, Alias;
 def dsym_dir : JoinedOrSeparate<["-"], "dsym-dir">,

It would be better to make `-module-dir` the main option and `-J` the alias. 
`-J` is only there for gfortran compatibility, not because it is a good name 
for the option.



Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
   /// {
+  Fortran::semantics::SemanticsContext () const { return 
*semantics_; }
 

`semanticsContext` would be a better name for this function.



Comment at: flang/include/flang/Frontend/CompilerInvocation.h:67
+  // of options.
+  std::string moduleDirJ_ = "."; 
+

`moduleDir_` would be a better name.



Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+  semantics_(new Fortran::semantics::SemanticsContext(*(new  
Fortran::common::IntrinsicTypeDefaultKinds()),*(new 
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
 

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.


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

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 updated this revision to Diff 320188.
arnamoy10 added a comment.

Added test for the driver-help, also contains all the changes that was done in 
the previous diff (Aliasing `-J` and `-module-dir`, changing data structures 
etc).


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

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/include-module.f90

Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -8,12 +8,28 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! 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
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
Index: flang/test/Flang-Driver/driver-help.f90
===
--- flang/test/Flang-Driver/driver-help.f90
+++ flang/test/Flang-Driver/driver-help.f90
@@ -26,6 +26,7 @@
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! HELP-NEXT: -help  Display available options
 ! HELP-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-NEXT: -JPut MODULE files in 'directory'
 ! HELP-NEXT: -o   Write output to 
 ! HELP-NEXT: -U  Undefine macro 
 ! HELP-NEXT: --version  Print version information
@@ -41,6 +42,7 @@
 ! HELP-FC1-NEXT: -E Only run the preprocessor
 ! HELP-FC1-NEXT: -help  Display available options
 ! HELP-FC1-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-FC1-NEXT: -JPut MODULE files in 'directory'
 ! HELP-FC1-NEXT: -o   Write output to 
 ! HELP-FC1-NEXT: -U  Undefine macro 
 ! HELP-FC1-NEXT: --version  Print version information
Index: 

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 updated this revision to Diff 320184.
arnamoy10 added a comment.

Addressing reviewers' comments with the following changes:

1. Aliasing of -module-dir and -J to avoid code duplication
2. Moving the code to set the module and search directories from 
`FrontendActions.cpp` to `CompilerInvocation.cpp`
3. Data structures updated/ variables added to separate Preprocessor options 
from Compiler Options
4. Added more test cases.


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

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/include-module.f90

Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -8,12 +8,28 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! 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
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -J %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs/module-dir -module-dir %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs/module-dir -J%S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s 2>&1 | FileCheck %s --check-prefix=INCLUDED
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
Index: flang/lib/Frontend/FrontendActions.cpp
===
--- flang/lib/Frontend/FrontendActions.cpp
+++ flang/lib/Frontend/FrontendActions.cpp
@@ -96,11 +96,9 @@
 
   auto {*ci.parsing().parseTree()};
 
-  // Prepare semantics
-  Fortran::semantics::SemanticsContext semanticsContext{
-  defaultKinds, features, ci.allCookedSources()};
+  // Prepare semantics 
   Fortran::semantics::Semantics semantics{
-  semanticsContext, parseTree, ci.parsing().cooked().AsCharBlock()};
+  ci.semaChecking(), parseTree, ci.parsing().cooked().AsCharBlock()};
 
   // Run semantic checks
   semantics.Perform();
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -183,6 +183,14 @@
 opts.searchDirectoriesFromDashI.emplace_back(currentArg->getValue());
 }
 
+/// Parses all semantic related arguments and populates the variables
+/// options accordingly.
+static void 

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@arnamoy10 , could you also add tests that show that `-J/-module-dir` is taken 
into account when deciding where to put module files? Thank you!


Repository:
  rG LLVM Github Monorepo

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

[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 this patch and for working on this! I have a few 
high-level suggestions (+ some inline comments):

**Q1**
`-module-dir` and `-J` in Options.td should be aliases - otherwise we need to 
duplicate some code. I've not used Aliases myself, but perhaps this example 

 could be helpful. You might discover that supporting 2 different spellings for 
one options is currently not possible. In such case we should start with one 
spelling.

**Q2**
Could you try moving your changes from FrontendActions.cpp to 
CompilerInvocation.cpp/CompilerInstance.cpp? Ideally, `CompilerInvocation` 
should encapsulate all compiler and langauge options relevant to the current 
invocation. Once we enter `FrontendActions::ExecuteAction`, all of them should 
be set and ready. This way `ExecuteAction` focuses on the action itself rather 
than setting it up. Also, adding `Fortran::semantics::SemanticsContext` to 
`CompilerInstance` could help with encapsulation.

**Q3**
What about help text for `-J`?

Thank you, this is looking really good and it's great to see more people 
working on the new driver!




Comment at: clang/include/clang/Driver/Options.td:1006-1007
   MarshallingInfoString>;
+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">,

As we are trying to follow `gfortran`, I suggest that we copy the help message 
from there:

```
$ gfortran --help=separate | grep '\-J'
  -J   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.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:24
+  Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U, options::OPT_I,
+options::OPT_J, options::OPT_module_dir});
 }

Are `-J` and `-module-dir` really preprocessor options? Wouldn't `OPT_J` and 
`OPT_module_dir` be better suited in `ConstructJob` (or some other method for 
other options)?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:92
 
-  const auto& D = C.getDriver();
+  const auto  = C.getDriver();
   // TODO: Replace flang-new with flang once the new driver replaces the

This is an unrelated change. I'm fine with this, but ideally as a separate NFC 
patch (otherwise the history is distorted).



Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:34
+  // Module directory specified by -J
+  std::string moduleDirJ;
+

IMHO `searchDirectoryFromDashJ` would be::
* more consistent (matches `searchDirectoryFromDashI`) 
* more accurate (internally this is only used for defining search directories)



Comment at: flang/include/flang/Parser/parsing.h:35
   std::vector searchDirectories;
+  std::string moduleDirectory;
   std::vector predefinitions;

This is merely module _search_ directory, right? Perhaps worth renaming as 
`moduleSearchDirectory`? 

Also, is it required by the parser? IIUC, it's only used when calling 
`set_moduleDirectory`, which is part of `Fortran::semantics::SemanticsContext`.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:186
+opts.moduleDirJ = currentArg->getValue();
+opts.searchDirectoriesFromDashI.emplace_back(currentArg->getValue());
+  }

`searchDirectoriesFromDashI` contains search directories specified with `-I`. 
If we add things from `-J` then the name is no longer valid :) One option is to 
rename the variable. Personally I think that we can skip this line.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:205
   const llvm::opt::OptTable  = clang::driver::getDriverOptTable();
-  const unsigned includedFlagsBitmask =
-  clang::driver::options::FC1Option;
+  const unsigned includedFlagsBitmask = clang::driver::options::FC1Option;
   unsigned missingArgIndex, missingArgCount;

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

[Arnamoy B via Phabricator via cfe-commits]

arnamoy10 created this revision.
arnamoy10 added reviewers: awarzynski, sscalpone, sameeranjoshi, SouraVX, 
tskeith, kiranktp, AMDChirag.
Herald added a subscriber: dang.
Herald added a reviewer: jansvoboda11.
arnamoy10 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add support for option -J/-module-dir in the new Flang driver.
This will allow for including module files in other directories, as the default 
search path is currently the working folder.  This also provides an option of 
storing the output module in the specified folder.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95448

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/PreprocessorOptions.h
  flang/include/flang/Parser/parsing.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/include-module.f90

Index: flang/test/Flang-Driver/include-module.f90
===
--- flang/test/Flang-Driver/include-module.f90
+++ flang/test/Flang-Driver/include-module.f90
@@ -1,4 +1,4 @@
-! Ensure argument -I works as expected with module files.
+! Ensure argument -I, -J and -module-dir works as expected with module files.
 ! The module files for this test are not real module files.
 
 ! REQUIRES: new-flang-driver
@@ -8,12 +8,20 @@
 !--
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
 
 !-
 ! FRONTEND FLANG DRIVER (flang-new -fc1)
 !-
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
 ! RUN: not %flang-new -fc1 -fsyntax-only -I %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -J %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs -I %S/Inputs/module-dir %s  2>&1 | FileCheck %s --check-prefix=INCLUDED
+! RUN: not %flang-new -fc1 -fsyntax-only -module-dir %S/Inputs %s  2>&1 | FileCheck %s --check-prefix=SINGLEINCLUDE
 
 !-
 ! EXPECTED OUTPUT FOR MISSING MODULE FILE
Index: flang/test/Flang-Driver/driver-help.f90
===
--- flang/test/Flang-Driver/driver-help.f90
+++ flang/test/Flang-Driver/driver-help.f90
@@ -26,6 +26,7 @@
 ! HELP-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! HELP-NEXT: -help  Display available options
 ! HELP-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-NEXT: -module-dir  Add to the list of directories to be searched by an USE statement
 ! HELP-NEXT: -o   Write output to 
 ! HELP-NEXT: -U  Undefine macro 
 ! HELP-NEXT: --version  Print version information
@@ -41,6 +42,7 @@
 ! HELP-FC1-NEXT: -E Only run the preprocessor
 ! HELP-FC1-NEXT: -help  Display available options
 ! HELP-FC1-NEXT: -IAdd directory to the end of the list of include search paths
+! HELP-FC1-NEXT: -module-dir  Add to the list of directories to be searched by an USE statement
 ! HELP-FC1-NEXT: -o   Write output to 
 ! HELP-FC1-NEXT: -U  Undefine macro 
 ! HELP-FC1-NEXT: --version  Print version information
Index: flang/test/Flang-Driver/driver-help-hidden.f90
===
--- flang/test/Flang-Driver/driver-help-hidden.f90
+++ flang/test/Flang-Driver/driver-help-hidden.f90
@@ -26,6 +26,7 @@
 ! CHECK-NEXT: -fno-color-diagnostics Disable colors in diagnostics
 ! CHECK-NEXT: -help Display available options
 ! CHECK-NEXT: -IAdd directory to the end of the list of include search paths
+! CHECK-NEXT: -module-dir  Add to the list of directories to be