[clang][cli] Let denormalizer decide how to render the option based on the option class

[jansvoboda11]

Summary

Before this patch, you needed to use AutoNormalizeEnumJoined whenever you wanted to denormalize joined enum.
Besides the naming confusion, this means the fact the option is joined is specified in two places: in the normalization multiclass and in the Joined<["-"], ...> multiclass.
This patch makes this work automatically, taking into account the OptionClass of options.

Also, the enum denormalizer now just looks up the spelling of the present enum case in a table and forwards it to the string denormalizer.

I also added more tests that exercise this.

[jansvoboda11]

Taking over this patch, as Daniel is no longer involved.

[jansvoboda11]

Rebase, add tests

[dexonsmith]

LGTM.

clang/include/clang/Driver/Options.td

def mt_migrate_directory : Separate<["-"], "mt-migrate-directory">,
   HelpText<"Directory for temporary files produced during ARC or ObjC migration">,
   MarshallingInfoString<"FrontendOpts.MTMigrateDir">;
 
 def arcmt_action_EQ : Joined<["-"], "arcmt-action=">, Flags<[CC1Option, NoDriverOption]>,
   HelpText<"The ARC migration action to take">, Values<"check,modify,migrate">,
   NormalizedValuesScope<"FrontendOptions">,
   NormalizedValues<["ARCMT_Check", "ARCMT_Modify", "ARCMT_Migrate"]>,
   MarshallingInfoString<"FrontendOpts.ARCMTAction", "ARCMT_None">,
-  AutoNormalizeEnumJoined;
+  AutoNormalizeEnum;
 
 def opt_record_file : Separate<["-"], "opt-record-file">,
   HelpText<"File name to use for YAML optimization record output">,
   MarshallingInfoString<"CodeGenOpts.OptRecordFile">;
 def opt_record_passes : Separate<["-"], "opt-record-passes">,
   HelpText<"Only record remark information for passes whose names match the given regular expression">;
 def opt_record_format : Separate<["-"], "opt-record-format">,
   HelpText<"The format used for serializing remarks (default: YAML)">;

clang/lib/Frontend/CompilerInvocation.cpp

 }
 
 /// The tblgen-erated code passes in a fifth parameter of an arbitrary type, but
 /// denormalizeSimpleFlags never looks at it. Avoid bloating compile-time with
 /// unnecessary template instantiations and just ignore it with a variadic
 /// argument.
 static void denormalizeSimpleFlag(SmallVectorImpl<const char *> &Args,
                                   const char *Spelling,
-                                  CompilerInvocation::StringAllocator, unsigned,
-                                  /*T*/...) {
+                                  CompilerInvocation::StringAllocator,
+                                  Option::OptionClass, unsigned, /*T*/...) {
   Args.push_back(Spelling);
 }
 
 namespace {
 template <typename T> struct FlagToValueNormalizer {
   T Value;
 
   Optional<T> operator()(OptSpecifier Opt, unsigned, const ArgList &Args,
if (const Arg *A = Args.getLastArg(Opt, OtherOpt)) {
       return A->getOption().matches(Opt) ? Value : OtherValue;
     }
     return None;
   };
 }
 
 static auto makeBooleanOptionDenormalizer(bool Value) {
   return [Value](SmallVectorImpl<const char *> &Args, const char *Spelling,
-                 CompilerInvocation::StringAllocator, unsigned, bool KeyPath) {
+                 CompilerInvocation::StringAllocator, Option::OptionClass,
+                 unsigned, bool KeyPath) {
     if (KeyPath == Value)
       Args.push_back(Spelling);
   };
 }
 
+static void denormalizeStringImpl(SmallVectorImpl<const char *> &Args,
+                                  const char *Spelling,
+                                  CompilerInvocation::StringAllocator SA,
+                                  Option::OptionClass OptClass, unsigned,
+                                  Twine Value) {
+  switch (OptClass) {
+  case Option::SeparateClass:
+  case Option::JoinedOrSeparateClass:
+    Args.push_back(Spelling);
+    Args.push_back(SA(Value));
+    break;
+  case Option::JoinedClass:
+    Args.push_back(SA(Twine(Spelling) + Value));
+    break;
+  default:
+    llvm_unreachable("Cannot denormalize an option with option class "
+                     "incompatible with string denormalization.");
+  }
+}
+
+template <typename T>
+static void
+denormalizeString(SmallVectorImpl<const char *> &Args, const char *Spelling,
+                  CompilerInvocation::StringAllocator SA,
+                  Option::OptionClass OptClass, unsigned TableIndex, T Value) {
+  denormalizeStringImpl(Args, Spelling, SA, OptClass, TableIndex, Twine(Value));
+}
+
 static Optional<SimpleEnumValue>
 findValueTableByName(const SimpleEnumValueTable &Table, StringRef Name) {
   for (int I = 0, E = Table.Size; I != E; ++I)
     if (Name == Table.Table[I].Name)
       return Table.Table[I];
 
   return None;
 }
static llvm::Optional<unsigned> normalizeSimpleEnum(OptSpecifier Opt,
   Diags.Report(diag::err_drv_invalid_value)
       << Arg->getAsString(Args) << ArgValue;
   return None;
 }
 
 static void denormalizeSimpleEnumImpl(SmallVectorImpl<const char *> &Args,
                                       const char *Spelling,
                                       CompilerInvocation::StringAllocator SA,
+                                      Option::OptionClass OptClass,
                                       unsigned TableIndex, unsigned Value) {
   assert(TableIndex < SimpleEnumValueTablesSize);
   const SimpleEnumValueTable &Table = SimpleEnumValueTables[TableIndex];
   if (auto MaybeEnumVal = findValueTableByValue(Table, Value)) {
-    Args.push_back(Spelling);
-    Args.push_back(MaybeEnumVal->Name);
+    denormalizeString(Args, Spelling, SA, OptClass, TableIndex,
+                      MaybeEnumVal->Name);
   } else {
     llvm_unreachable("The simple enum value was not correctly defined in "
                      "the tablegen option description");
   }
 }
 
 template <typename T>
 static void denormalizeSimpleEnum(SmallVectorImpl<const char *> &Args,
                                   const char *Spelling,
                                   CompilerInvocation::StringAllocator SA,
+                                  Option::OptionClass OptClass,
                                   unsigned TableIndex, T Value) {
-  return denormalizeSimpleEnumImpl(Args, Spelling, SA, TableIndex,
+  return denormalizeSimpleEnumImpl(Args, Spelling, SA, OptClass, TableIndex,
                                    static_cast<unsigned>(Value));
 }
 
-static void denormalizeSimpleEnumJoined(SmallVectorImpl<const char *> &Args,
-                                        const char *Spelling,
-                                        CompilerInvocation::StringAllocator SA,
-                                        unsigned TableIndex, unsigned Value) {
-  assert(TableIndex < SimpleEnumValueTablesSize);
-  const SimpleEnumValueTable &Table = SimpleEnumValueTables[TableIndex];
-  if (auto MaybeEnumVal = findValueTableByValue(Table, Value))
-    Args.push_back(SA(Twine(Spelling) + MaybeEnumVal->Name));
-  else
-    llvm_unreachable("The simple enum value was not correctly defined in "
-                     "the tablegen option description");
-}
-
 static Optional<std::string> normalizeString(OptSpecifier Opt, int TableIndex,
                                              const ArgList &Args,
                                              DiagnosticsEngine &Diags) {
   auto *Arg = Args.getLastArg(Opt);
   if (!Arg)
     return None;
   return std::string(Arg->getValue());
 }
 
-static void denormalizeString(SmallVectorImpl<const char *> &Args,
-                              const char *Spelling,
-                              CompilerInvocation::StringAllocator SA, unsigned,
-                              Twine Value) {
-  Args.push_back(Spelling);
-  Args.push_back(SA(Value));
-}
-
-template <typename T,
-          std::enable_if_t<!std::is_convertible<T, Twine>::value &&
-                               std::is_constructible<Twine, T>::value,
-                           bool> = false>
-static void denormalizeString(SmallVectorImpl<const char *> &Args,
-                              const char *Spelling,
-                              CompilerInvocation::StringAllocator SA,
-                              unsigned TableIndex, T Value) {
-  denormalizeString(Args, Spelling, SA, TableIndex, Twine(Value));
-}
-
 template <typename IntTy>
 static Optional<IntTy> normalizeStringIntegral(OptSpecifier Opt, int,
                                                const ArgList &Args,
                                                DiagnosticsEngine &Diags) {
   auto *Arg = Args.getLastArg(Opt);
   if (!Arg)
     return None;
   IntTy Res;
#define OPTION_WITH_MARSHALLING(                                               \
     IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
     TABLE_INDEX)                                                               \
   if ((FLAGS)&options::CC1Option) {                                            \
     [&](const auto &Extracted) {                                               \
       if (ALWAYS_EMIT ||                                                       \
           (Extracted !=                                                        \
            static_cast<decltype(this->KEYPATH)>(                               \
                (IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE))))          \
-        DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);              \
+        DENORMALIZER(Args, SPELLING, SA, Option::KIND##Class, TABLE_INDEX,     \
+                     Extracted);                                               \
     }(EXTRACTOR(this->KEYPATH));                                               \
   }
 
 #include "clang/Driver/Options.inc"
 #undef OPTION_WITH_MARSHALLING
 }
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>

clang/unittests/Frontend/CompilerInvocationTest.cpp

TEST_F(CommandLineTest, CanGenerateCC1CommandLineSeparateRequiredAbsent) {
   ASSERT_FALSE(Diags->hasErrorOccurred());
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
 
   // Triple should always be emitted even if it is the default
   ASSERT_THAT(GeneratedArgs, Contains(StrEq(DefaultTriple.c_str())));
 }
 
-TEST_F(CommandLineTest, CanGenerateCC1CommandLineSeparateEnumNonDefault) {
+TEST_F(CommandLineTest, SeparateEnumNonDefault) {
   const char *Args[] = {"-mrelocation-model", "static"};
 
   CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
 
   ASSERT_FALSE(Diags->hasErrorOccurred());
+  ASSERT_EQ(Invocation.getCodeGenOpts().RelocationModel, Reloc::Model::Static);
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
 
   // Non default relocation model.
+  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-mrelocation-model")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("static")));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-mrelocation-model=static"))));
 }
 
-TEST_F(CommandLineTest, CanGenerateCC1COmmandLineSeparateEnumDefault) {
+TEST_F(CommandLineTest, SeparateEnumDefault) {
   const char *Args[] = {"-mrelocation-model", "pic"};
 
   CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
 
   ASSERT_FALSE(Diags->hasErrorOccurred());
+  ASSERT_EQ(Invocation.getCodeGenOpts().RelocationModel, Reloc::Model::PIC_);
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
 
   // Default relocation model.
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-mrelocation-model"))));
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("pic"))));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-mrelocation-model=pic"))));
+}
+
+TEST_F(CommandLineTest, JoinedEnumNonDefault) {
+  const char *Args[] = {"-fobjc-dispatch-method=non-legacy"};
+
+  CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
+
+  ASSERT_FALSE(Diags->hasErrorOccurred());
+  ASSERT_EQ(Invocation.getCodeGenOpts().getObjCDispatchMethod(),
+            CodeGenOptions::NonLegacy);
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs,
+              Contains(StrEq("-fobjc-dispatch-method=non-legacy")));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fobjc-dispatch-method="))));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("non-legacy"))));
+}
+
+TEST_F(CommandLineTest, JoinedEnumDefault) {
+  const char *Args[] = {"-fobjc-dispatch-method=legacy"};
+
+  CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
+
+  ASSERT_FALSE(Diags->hasErrorOccurred());
+  ASSERT_EQ(Invocation.getCodeGenOpts().getObjCDispatchMethod(),
+            CodeGenOptions::Legacy);
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs,
+              Not(Contains(StrEq("-fobjc-dispatch-method=legacy"))));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fobjc-dispatch-method="))));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("legacy"))));
 }
 
 // Tree of boolean options that can be (directly or transitively) implied by
 // their parent:
 //
 //   * -cl-unsafe-math-optimizations
 //     * -cl-mad-enable
 //     * -menable-unsafe-fp-math

llvm/include/llvm/Option/OptParser.td

 class Normalizer<code normalizer> { code Normalizer = normalizer; }
 class Denormalizer<code denormalizer> { code Denormalizer = denormalizer; }
 class NormalizedValuesScope<code scope> { code NormalizedValuesScope = scope; }
 class NormalizedValues<list<code> definitions> { list<code> NormalizedValues = definitions; } 
 class AutoNormalizeEnum {
   code Normalizer = "normalizeSimpleEnum";
   code Denormalizer = "denormalizeSimpleEnum";
 }
-class AutoNormalizeEnumJoined : AutoNormalizeEnum {
-  code Denormalizer = "denormalizeSimpleEnumJoined";
-}
 class ValueMerger<code merger> { code ValueMerger = merger; }
 class ValueExtractor<code extractor> { code ValueExtractor = extractor; }
 
 // Predefined options.
 
 // FIXME: Have generator validate that these appear in correct position (and
 // aren't duplicated).
 def INPUT : Option<[], "<input>", KIND_INPUT>;
 def UNKNOWN : Option<[], "<unknown>", KIND_UNKNOWN>;