Fix Review comments:

-Update internals-manual

-Update document generator comment, clarify we intend to document as if
it is a 'select'

-Add test coverage for other modifiers using '<'

-Add additional suggested test coverage

Author: erichkeane

clang/docs/InternalsManual.rst

@@ -276,6 +276,21 @@ Description:
   diagnostic instead of having to do things textually.  The selected string
   does undergo formatting.
 
+**"enum_select format**
+
+Example:
+  ``unknown frobbling of a %enum_select<FrobbleKind>{%VarDecl{variable declaration}|%FuncDecl{function declaration}}0 when blarging``
+Class:
+  Integers
+Description:
+  This format specifier is used exactly like a ``select`` specifier, except it
+  additionally generates a namespace, enumeration, and enumerator list based on
+  the format string given. In the above case, a namespace is generated named
+  ``FrobbleKind`` that has a anonymous enumeration with the enumerators
+  ``VarDecl`` and ``FuncDecl`` which correspond to the values 0 and 1. This
+  permits a clearer use of the ``Diag`` in source code, as the above could be
+  called as: ``Diag(Loc, diag::frobble) << diag::FrobbleKind::VarDecl``.
+
 **"plural" format**
 
 Example:

clang/test/TableGen/select-enum-errors.td

@@ -1,16 +1,45 @@
-// RUN: clang-tblgen --gen-clang-diags-enums -I%S %s 2>&1 | FileCheck %s
+// RUN: not clang-tblgen --gen-clang-diags-enums -DERROR1 -I%S %s 2>&1 | FileCheck %s --check-prefixes=CHECK,C1
+// RUN: not clang-tblgen --gen-clang-diags-enums -DERROR2 -I%S %s 2>&1 | FileCheck %s --check-prefixes=CHECK,C2
+// RUN: not clang-tblgen --gen-clang-diags-enums -DERROR3 -I%S %s 2>&1 | FileCheck %s --check-prefixes=CHECK,C3
+// RUN: not clang-tblgen --gen-clang-diags-enums -DERROR4 -I%S %s 2>&1 | FileCheck %s --check-prefixes=CHECK,C4
 include "DiagnosticBase.inc"
 
+// No real reason to diagnose these, the namespace generated as the
+// 'enumeration' name will never conflict with the enumerator.
+def EnumerationEnumeratorDupe : Error<"%enum_select<Matchy>{%Matchy{haha}}0">;
+
+// Enumerator values aren't required, though this does seem kind of silly/not
+// particularly useful?
+def NoEnumerators : Error<"%enum_select<Name>{foo|bar|baz}0">;
+
 def DupeNames1 : Error<"%enum_select<DupeName>{}0">;
 def DupeNames2 : Error<"%enum_select<DupeName>{}0">;
 // CHECK: error: Duplicate enumeration name 'DupeName'
 // CHECK-NEXT: def DupeNames2
 // CHECK: note: Previous diagnostic is here
 // CHECK-NEXT: def DupeNames1
 
-def DupeValue : Error<"%enum_select<DupeValue>{%Name{V1}|%Name{V2}}0">;
-// CHECK: error: Duplicate enumerator name 'Name'
+def DupeValue : Error<"%enum_select<DupeValue>{%DName{V1}|%DName{V2}}0">;
+// CHECK: error: Duplicate enumerator name 'DName'
 
+#ifdef ERROR1
 def EnumValNotExpected : Error<"%enum_select{V1|%Val2{V2}}0">;
-// CHECK: expected '<' after enum_select
+// C1: expected '<' after enum_select
+#endif
+
+#ifdef ERROR2
+def SelectWithArrow : Error<"%select<Something>{V1|%Val2{V2}}0">;
+// C2: modifier '<' syntax not valid with %select
+#endif
+
+#ifdef ERROR3
+// Missing closing > after the name of the enumeration
+def MissingClosing : Error<"%enum_select<MissingClosingName{}0">;
+// C3: expected '>' while parsing %enum_select
+#endif
 
+#ifdef ERROR4
+// Missing { after the name of an enumerator
+def MissingTextAfterEnumerator: Error<"%enum_select<Name>{%OtherName|foo}0">;
+// C4: expected '{' while parsing %enum_select
+#endif

clang/utils/TableGen/ClangDiagnosticsEmitter.cpp

@@ -910,7 +910,9 @@ struct DiagTextDocPrinter : DiagTextVisitor<DiagTextDocPrinter> {
   }
 
   void VisitEnumSelect(EnumSelectPiece *P) {
-    // For now, document this as if it were a 'select'.
+    // Document this as if it were a 'select', which properly prints all of the
+    // options correctly in a readable/reasonable manner. There isn't really
+    // anything valuable we could add to readers here.
     VisitSelect(P);
   }
 
@@ -1120,7 +1122,8 @@ Piece *DiagnosticTextBuilder::DiagText::parseDiagText(StringRef &Text,
     };
 
     if (ModType != MT_EnumSelect && Text[0] == '<')
-      Builder.PrintFatalError("modifier '<' syntax not valid with " + Modifier);
+      Builder.PrintFatalError("modifier '<' syntax not valid with %" +
+                              Modifier);
 
     switch (ModType) {
     case MT_Unknown: