[flang-commits] [PATCH] D106727: [flang] Produce proper "preprocessor output" for -E option

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

Thank you for updating the new driver and fixing the tests. Everything builds fine and tests pass. Tested with the new driver. Since you are adding this in the new driver too, I suggest that for consistency sake we rename `-fdebug-dump-cooked-chars` . Let me elaborate.

All options in the form of `-fdebug-dump-<something>` correspond to an action and have a dedicated instance of `FrontendAction` (in `FrontendActions.h`). `-fdebug-dump-cooked-chars` is a feature option that configures `-E` (i.e. preprocessing) rather than an action in itself. Indeed, you tweaked `PrintPreprocessedAction` rather than created a new action, which makes a lot of sense.  As `-fdebug-dump-cooked-chars` is not an action, there are a few changes here that are not required (see inline comments). As for the spelling itself, `-fdebug-dump-cooked-chars` will not dump anything on its own. It needs to be paired with `-E`.  So the name is a bit misleading. Perhaps we could have something like `-fcooked-only` or `-fno-reformat`? Naming is hard, I have no particular preferences.

Apologies for not bringing this up earlier. In `f18`, we don't differentiate between "action" options and "feature" options, so it doesn't matter that much. But in `flang-new` it becomes more relevant. Let me know if you'd like me to go into more detail with respect to action/non-action flags.



================
Comment at: clang/include/clang/Driver/Options.td:4535-4536
   HelpText<"Run the InputOuputTest action. Use for development and testing only.">;
+def fdebug_dump_cooked_chars : Flag<["-"], "fdebug-dump-cooked-chars">, Group<Preprocessor_Group>,
+  HelpText<"Dump the cooked character stream in -E mode">;
 def fdebug_unparse_no_sema : Flag<["-"], "fdebug-unparse-no-sema">, Group<Action_Group>,
----------------
This is the decision point (action vs non-action option):
* `Group<Preprocessor_Group>` --> `-fdebug-dump-cooked-chars` is a preprocessor flag (i.e. tweaks the behavior of `-E`)
* `Group<Action_Group>` --> `-fdebug-dump-cooked-chars` is an action flag

Just highlighting. I think that `Preprocessor_Group` is exactly what we want here.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31-33
+  /// -fdebug-dump-cooked-chars
+  DebugDumpCookedChars,
+
----------------
Not needed, `-fdebug-dump-cooked-chars` is not an action.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:255-276
+public:
+  // -P: Suppress #line directives in -E output
+  bool noLineDirectives() const { return noLineDirectives_; }
+  FrontendOptions &set_noLineDirectives(bool yes = true) {
+    noLineDirectives_ = yes;
+    return *this;
+  }
----------------
Do you think that we need accessors and mutators here? We've kept `FrontendOptions` rather basic - it's just a plain aggregate. There's no intention to make it any more "clever". Keeping everything "public" allows us to have a rather basic and concise interface. 

If we do add member methods for `noLineDirectives_` and `noLineDirectives_`, then perhaps we should do it for all member variables? But then we'll bloat this class.

Also, we've been using bitfields rather than `bool`s for this class to keep it slim. Could we stick to bitfields for consistency?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:122-124
+      opts.set_noLineDirectives(args.hasArg(clang::driver::options::OPT_P));
+      opts.set_dumpCookedChars(
+          args.hasArg(clang::driver::options::OPT_fdebug_dump_cooked_chars));
----------------
In this switch statement we only decide _which_ action to run. In this case, it's `PrintPreprocessedInput`. 

Preprocessor options are set in [[ https://github.com/llvm/llvm-project/blob/43e45f0ec920b45d6073c0aff47597c44948f52c/flang/lib/Frontend/CompilerInvocation.cpp#L345-L374 | parsePreprocessorArgs ]]. Could you move this code there?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:126-127
+      break;
+    case clang::driver::options::OPT_fdebug_dump_cooked_chars:
+      opts.programAction_ = DebugDumpCookedChars;
       break;
----------------
This case is not needed, `-fdebug-dump-cooked-chars` is not an action.


================
Comment at: flang/lib/Parser/parsing.cpp:57-58

----------------
This is nly required for fixed-form, but not for free-form. That's fine, but it's worth adding a comment that this is intentional. For example:
```
// Make sure that the first 6 columns are formatted correctly. We focus on the standard Fortran fixed form, which is also valid free form.
```
My future self will be grateful.


================
Comment at: flang/test/Preprocessing/dash-E.F90:1
+! RUN: %flang -E %s 2>&1 | FileCheck %s
+#define ADD(x,y) (x)+(y)
----------------
How about statement labels? That's still not tested.


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

https://reviews.llvm.org/D106727