[PATCH] D95366: [clang][cli] Generate and round-trip preprocessor options

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3093
+
+static bool ParsePreprocessorArgs(PreprocessorOptions &Opts, ArgList &Args,
                                   DiagnosticsEngine &Diags,
----------------
dexonsmith wrote:
> Can we name this differently, so it's obvious which is being called without looking at the argument list? I suggest `ParsePreprocessorArgsImpl` for this one, since it's doing the actual parsing.
I thought about it and decided to keep it the same. We'd be renaming it back to `ParsePreprocessorArgs` in a few weeks, when we round-trip the whole CompilerInvocation at once and the need for the fine-grained interposed functions disappears.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3215-3216
+
+  return RoundTrip(Parse, Generate, Swap, Res, Args, Diags,
+                   "PreprocessorOptions");
 }
----------------
dexonsmith wrote:
> Have you considered just defining the lambdas inline in the call to `RoundTrip`? I'm fine either way, but the way clang-format tends to clean this up seems pretty readable to me, and the names don't really add much value since they match the functions being called. Up to you.
In this case, where one of the lambdas contains a long comment, I suggest keeping it separate. It's easier to read that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95366