[PATCH] D93453: [flang][driver] Add support for `-I`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I left a few nits, but otherwise LGTM!



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:25
   Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U});
+  Args.AddAllArgs(CmdArgs, options::OPT_I);
 }
----------------
[nit] This probably can be merged with the line above.


================
Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:13
   std::vector<std::pair<std::string, bool /*isUndef*/>> Macros;
+  std::vector<std::string> SearchDirectoriesFromI;
 
----------------
awarzynski wrote:
> [nit] I'd be tempted to be more explicit here, e.g. `SearchDirectoriesFromDashI;`.
I think that once the new driver supports more search-path related options (rather than just `-I`), it will make sense to extract these into a separate class. That's not really needed just now, but it's probably worth adding a comment. Perhaps something like this:
```
// TODO: When adding support for more options related to search paths, consider collecting them in a separate aggregate. For now we keep it here as there is no point creating a class for just one field.
```


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:264-267
+  for (const auto &searchDirectoryFromI :
+      preprocessorOptions.searchDirectoriesFromDashI) {
+    fortranOptions.searchDirectories.emplace_back(searchDirectoryFromI);
+  }
----------------
If these are `std::vector`s (which I believe is the case), then you can simplify:
```
fortranOptions.searchDirectories.insert(
    fortranOptions.searchDirectories.end(), 
    preprocessorOptions.searchDirectoriesFromDashI.begin(),
    preprocessorOptions.searchDirectoriesFromDashI.end());
```


================
Comment at: flang/test/Flang-Driver/driver-help.f90:27
 ! HELP-NEXT: -help                  Display available options
+! HELP-NEXT: -I <dir>               Add directory to include search path. If there are multiple -I options, these directories are searched in the order they are given before the standard system directories are searched. If the same directory is in the SYSTEM include search paths, for example if also specified with -isystem, the -I option will be ignored
 ! HELP-NEXT: -o <file>              Write output to <file>
----------------
The help message for `-I` is _very_ long :) I wonder, perhaps you could just keep `Add directory to include search path.`?

The purpose of this test file is to verify that `-help` only displays the supported options. Technically speaking only `-I` is needed for that :) (but that could be _too_ short)


================
Comment at: flang/test/Flang-Driver/include_header.f90:33
+
+#include <included.h>
+#ifdef X
----------------
[nit] IMO the naming is a bit unfortunate, e.g. there's `include` and `included` on one line. Perhaps: `basic-test-header.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93453