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 <https://github.com/llvm/llvm-project/blob/21bfd068b32ece1c6fbc912208e7cd1782a8c3fc/clang/include/clang/Driver/Options.td#L1095-L1097> 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<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">, ---------------- 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. ================ 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 &D = 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<std::string> searchDirectories; + std::string moduleDirectory; std::vector<Predefinition> 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 &opts = clang::driver::getDriverOptTable(); - const unsigned includedFlagsBitmask = - clang::driver::options::FC1Option; + const unsigned includedFlagsBitmask = clang::driver::options::FC1Option; unsigned missingArgIndex, missingArgCount; ---------------- Unrelated change ================ Comment at: flang/lib/Frontend/FrontendActions.cpp:104 + // Also add the search directories + if (!ci.fortranOpts().moduleDirectory.empty()) + semanticsContext.set_moduleDirectory(ci.fortranOpts().moduleDirectory) ---------------- I think that we can safely set a default value for `moduleDirectory` and skip this check. From https://gcc.gnu.org/onlinedocs/gfortran/Directory-Options.html: ``` The default is the current directory. ``` ================ 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 ---------------- 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? 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
