[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<gfortran_Group>;
-def J : JoinedOrSeparate<["-"], "J">, Flags<[RenderJoined]>, Group<gfortran_Group>;
 def cpp : Flag<["-"], "cpp">, Group<gfortran_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<gfortran_Group>;
+def module_dir : Separate<["-"], "module-dir">, Flags<[FlangOption,FC1Option]>, MetaVarName<"<dir>">, Alias<J>;
 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 &semaCtxt) {
+  auto &fortranOptions = fortranOpts();
----------------
awarzynski wrote:
> The argument is not needed here, is it? You could just write:
> ```
> auto &semaCtx = 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