[PATCH] D97457: [flang][driver] Add `-fdebug-dump-parsing-log`

[Chirag Khandelwal via Phabricator via cfe-commits]

AMDChirag added inline comments.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:88-95
+// Tweak the frontend configuration based on the frontend action
+static void setUpFrontendBasedOnAction(FrontendOptions &opts) {
+  assert(opts.programAction_ != Fortran::frontend::InvalidAction &&
+      "Fortran frontend action not set!");
+
+  if (opts.programAction_ == DebugDumpParsingLog)
+    opts.instrumentedParse_ = true;
----------------
awarzynski wrote:
> AMDChirag wrote:
> > Will this function be extended in the future?
> > If not, an entirely separate function for a couple statements seems rather overkill.
> > Will this function be extended in the future?
> 
> Yes.
> 
> Ideally we'd want _features_ and _actions_ to be orthogonal and to be controlled by dedicated flags. In this patch, we are adding an _action_ flag that toggles a _feature_ option. I think that that's a bit counter-intuitive, but not uncommon or unavoidable long-term.
> 
> Instead of adding comments, I prefer to introduce a dedicated method for this logic. It will make it easier for us to keep it in one place when new options like this are added.  Also, `ParseFrontendArgs` is already quite long and is only going to get longer.
> 
> Having said all that, we may decide in the future that there's a better way to split this logic. For now I mostly want to avoid extending `ParseFrontendArgs` too much. 
Fair, thank you for explaining!

In such case, shouldn't the function call `setUpFrontendBasedOnAction(opts);` be placed after all the fields of the `opts` variable have been set?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97457