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