[PATCH] D93401: [flang][driver] Add support for `-D`, `-U`

[Andrzej Warzynski via Phabricator via cfe-commits]

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