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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
