[clang][cli] NFC: Make parsing macro reusable

[jansvoboda11]

Summary

This is necessary for a future patch, where we start using this macro in another function.

[jansvoboda11]

Add newline at the end of file, simplify macro forwarding

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

3009–3010

One concern I have with this change is that the macro is using local variable names; it really would be better to have the macro content local to the function(s) where the variables are defined.

Can you give more context about why this has to be called from another place? Maybe there's another way to solve the problem.

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

3009–3010

I've provided more context here: D93701.

We can solve the problem with implicit use of local variables inside the macro by promoting them to macro parameters.
This will make the forwarding call from OPTION_WITH_MARSHALLING to PARSE_OPTION_WITH_MARSHALLING longer, as it will need to spell out all parameters, but it would solve your concern I think.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

3009–3010

I like that idea. That approach also allows you to drop unused parameters entirely in PARSE_OPTIONS_WITH_MARSHALLING (only forward the parameters that get used).

3310–3311

I like the idea of #undef'ing these macros to make their scope of use clear, but I'm not sure doing it at the end of file is adding a lot of value.

Is there a way of moving the call sites for each to be next to each other in the file, so you can #define and then #undef in a more limited scope? E.g.,

// maybe other content...

#define PARSE_OPTION_WITH_MARSHALLING(...) ...
void f1(...) { /* use PARSE_OPTION_WITH_MARSHALLING */ }
void f2(...) { /* use PARSE_OPTION_WITH_MARSHALLING */ }
#undef PARSE_OPTION_WITH_MARSHALLING

// maybe other content...

#define GENERATE_OPTION_WITH_MARSHALLING(...) ...
void f3(...) { /* use GENERATE_OPTION_WITH_MARSHALLING*/ }
void f4(...) { /* use GENERATE_OPTION_WITH_MARSHALLING*/ }
#undef GENERATE_OPTION_WITH_MARSHALLING

// maybe other content...

(If so, the code movement should be done in a separate NFC prep commit...)

[jansvoboda11]

Pass local variables to macros explicitly, rebase on top of prep patch.

[jansvoboda11]

Rebase

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

3009–3010

I've dropped the unused parameters for the PARSE_ macro. D84673 does the same for the GENERATE_ macro.

3310–3311

Yeah, limiting the scope where the macro is defined would be nice. I've moved things around and we'll be able to keep the GENERATE_ macro local to the function, while the PARSE_ macro will span only the two functions that'll make use of it.

[dexonsmith]

LGTM.

clang/lib/Frontend/CompilerInvocation.cpp

if (BadChar != Prefix.end() || !isLetter(Prefix[0])) {
       Success = false;
       Diags.Report(diag::err_drv_invalid_value) << "-verify=" << Prefix;
       Diags.Report(diag::note_drv_verify_prefix_spelling);
     }
   }
   return Success;
 }
 
-bool CompilerInvocation::parseSimpleArgs(const ArgList &Args,
-                                         DiagnosticsEngine &Diags) {
-  bool Success = true;
-
-#define OPTION_WITH_MARSHALLING(                                               \
-    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
-    HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
-    DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
-    MERGER, EXTRACTOR, TABLE_INDEX)                                            \
+#define PARSE_OPTION_WITH_MARSHALLING(ARGS, DIAGS, SUCCESS, ID, FLAGS, PARAM,  \
+                                      SHOULD_PARSE, KEYPATH, DEFAULT_VALUE,    \
+                                      IMPLIED_CHECK, IMPLIED_VALUE,            \
+                                      NORMALIZER, MERGER, TABLE_INDEX)         \
   if ((FLAGS)&options::CC1Option) {                                            \
     this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);                      \
     if (IMPLIED_CHECK)                                                         \
       this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);                    \
     if (SHOULD_PARSE)                                                          \
       if (auto MaybeValue =                                                    \
-              NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags, Success))         \
+              NORMALIZER(OPT_##ID, TABLE_INDEX, ARGS, DIAGS, SUCCESS))         \
         this->KEYPATH = MERGER(                                                \
             this->KEYPATH, static_cast<decltype(this->KEYPATH)>(*MaybeValue)); \
   }
 
+bool CompilerInvocation::parseSimpleArgs(const ArgList &Args,
+                                         DiagnosticsEngine &Diags) {
+  bool Success = true;
+
+#define OPTION_WITH_MARSHALLING(                                               \
+    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
+    HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
+    DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
+    MERGER, EXTRACTOR, TABLE_INDEX)                                            \
+  PARSE_OPTION_WITH_MARSHALLING(Args, Diags, Success, ID, FLAGS, PARAM,        \
+                                SHOULD_PARSE, KEYPATH, DEFAULT_VALUE,          \
+                                IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER,      \
+                                MERGER, TABLE_INDEX)
 #include "clang/Driver/Options.inc"
 #undef OPTION_WITH_MARSHALLING
 
   return Success;
 }
 
+#undef PARSE_OPTION_WITH_MARSHALLING
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticsEngine *Diags,
                                 bool DefaultDiagColor) {
   Optional<DiagnosticsEngine> IgnoringDiags;
   if (!Diags) {
     IgnoringDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                           new IgnoringDiagConsumer());
     Diags = &*IgnoringDiags;
if (Arg *A = Args.getLastArg(options::OPT_target_sdk_version_EQ)) {
     llvm::VersionTuple Version;
     if (Version.tryParse(A->getValue()))
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
     else
       Opts.SDKVersion = Version;
   }
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                         ArrayRef<const char *> CommandLineArgs,
                                         DiagnosticsEngine &Diags,
                                         const char *Argv0) {
   bool Success = true;
 
   // Parse the arguments.
   const OptTable &Opts = getDriverOptTable();
   const unsigned IncludedFlagsBitmask = options::CC1Option;
for (const auto &File : CI.getHeaderSearchOpts().VFSOverlayFiles) {
     if (!FS) {
       Diags.Report(diag::err_invalid_vfs_overlay) << File;
       continue;
     }
 
     Result = FS;
   }
   return Result;
 }