awarzynski added a comment.
@FarisRehman, thank you for working on this! This looks really good and I think
that it's almost ready. I did leave a few comments, but most are a matter of
style and should be easy to address.
================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:70-71
+ types::ID InputType = Input.getType();
+ if (types::getPreprocessedType(InputType) != types::TY_INVALID) {
+ Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U});
+ }
----------------
Clang deals with the preprocessor options in a dedicated method,
`AddPreprocessingOptions`. See here:
https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/clang/lib/Driver/ToolChains/Clang.cpp#L1077
I feel that it might be a good idea to follow that. The preprocessor options
(and the corresponding logic) are bound to grow, so we may as well add a
dedicated method sooner rather than later.
================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:73
+ }
+ Args.ClaimAllArgs(options::OPT_D);
+
----------------
AFAIK, when you call `AddAllArgs` the corresponding options are eventually
_claimed_:
https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/llvm/lib/Option/ArgList.cpp#L112
So, IIUC, this line is not needed.
================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:77
- const auto& D = C.getDriver();
// TODO: Replace flang-new with flang once the new driver replaces the
----------------
This change is not needed, is it?
================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:32
+ Fortran::frontend::PreprocessorOptions preprocessorOpts_;
+
----------------
1. Could you add a comment explaining what this is for?
2. Could you follow the current semantics used in the file and make it a
`std::shared_ptr`? Long term we can expect `PreprocessorOptions` to grow big
and at that point we wouldn't want to copy it too often (especially when
compiling a lot of files).
================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:84
// compiler driver options in libclangDriver.
- void SetDefaultFortranOpts();
+ void SetFortranOpts();
};
----------------
IIUC, you are renaming this method because it is finally translating the user
provided options (i.e. preprocessorOpts()) into Frontend options (i.e.
`fortranOptions`). However, with your changes this method:
* sets some _sane_ predefined defaults
* adds some stuff from the user (the preprocessor defines)
So it's somewhere in between `SetDefaultFortranOpts` and `SetFortranOpts` (i.e.
both names are _almost_ accurate). Perhaps splitting this into two methods
would be better? E.g.:
* `SetDefaultFortranOpts` (leave as is)
* `SetFortranOpts` (implements new functionality, to replace
`SetDefaultFortranOpts` eventually)
================
Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:1
+#ifndef LLVM_FLANG_PREPROCESSOROPTIONS_H
+#define LLVM_FLANG_PREPROCESSOROPTIONS_H
----------------
Missing file header: https://llvm.org/docs/CodingStandards.html#file-headers
================
Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:8
+
+/// PreprocessorOptions - This class is used for passing the various options
+/// used in preprocessor initialization to the parser options.
----------------
[nit] No need to repeat the name of the class.
================
Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:12
+public:
+ std::vector<std::pair<std::string, bool /*isUndef*/>> Macros;
+
----------------
[nit] IMO this would be preferred : `/*isUndef=*/bool`
(https://llvm.org/docs/CodingStandards.html#comment-formatting) (currently you
have `bool /*isUndef*/`)
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:157
+static void ParsePreprocessorArgs(
+ Fortran::frontend::PreprocessorOptions &opts, llvm::opt::ArgList &args) {
----------------
I couldn't find any notes on naming static functions here:
https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md. This
means that we fallow this:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.
So, `parsePreprocessorArgs` instead of `ParsePreprocessorArgs`.
Also, could you add a comment briefly explaining what the method is for? E.g.
`Parses all preprocessor input arguments and populates the preprocessor options
accordingly`? Ideally doxygen :) See example here:
https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/flang/include/flang/Frontend/CompilerInstance.h#L158-L163
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:160
+ // Add macros from the command line.
+ for (const auto *A : args.filtered(
+ clang::driver::options::OPT_D, clang::driver::options::OPT_U)) {
----------------
Variable names should start with lower case.
[nit] Perhaps `currentArg` instead of `A` for the sake of being explicit? Or
`curArg`?
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:205
+/// Collect the macro definitions provided by the given preprocessor
+/// options into the parser options.
+void collectMacroDefinitions(
----------------
This is not quite true, because you're collecting them into preprocessor
options :)
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:206
+/// options into the parser options.
+void collectMacroDefinitions(
+ Fortran::parser::Options &opts, const PreprocessorOptions &ppOpts) {
----------------
`static void` instead (we don't need this method to be visible outside this
translation unit).
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:208
+ Fortran::parser::Options &opts, const PreprocessorOptions &ppOpts) {
+ for (unsigned I = 0, N = ppOpts.Macros.size(); I != N; ++I) {
+ llvm::StringRef Macro = ppOpts.Macros[I].first;
----------------
All variables here have to be lower case:.
================
Comment at: flang/test/Flang-Driver/macro.f90:1
+! Ensure arguments -D and -U work as expected.
+
----------------
There are no tests for e.g. `-DSOME_VAR=SOME_VAL`. It would be good to test
that `SOME_VAL` was parsed correctly.
[nit[ Perhaps `macro_def_undef.f90` would be more descriptive? (re filename).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93401/new/
https://reviews.llvm.org/D93401
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
