[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this is great.

In https://reviews.llvm.org/D46740#1096859, @EricWF wrote:

> Turns out we have to fully parse the diagnostic text in order to substitute modifier indexes, which is a bit of a complication. This patch does exactly that.


I like that you were able to reuse the existing parsing code for documentation generation here.

In https://reviews.llvm.org/D46740#1096868, @EricWF wrote:

> @rsmith Can select indexes be negative? What's the correct type to represent them? Existing code seems to use `int`, but the LLVM style seems to suggest `unsigned` is more appropriate.


No, select indexes must be in the range 0..<number of options-1>. I don't think we actually have a convention of using `unsigned` for integers that can't be negative, and I suspect that if we thought about a policy we'd pick the opposite one :)

In https://reviews.llvm.org/D46740#1096972, @EricWF wrote:

> @rsmith, @rjmccall: This change set is a lot larger now. Is it appropriate to commit the Sema cleanup with this patch?


I would ideally like to see one substitution added, and one set of diagnostics converted, as part of this patch, and for other diagnostics to be converted in separate changes after that.



================
Comment at: docs/InternalsManual.rst:337
+    def note_ovl_candidate : Note<
+      "candidate %{select_ovl_candidate}3,2,1 not viable">;
+
----------------
Missing the `sub` specifier here?


================
Comment at: docs/InternalsManual.rst:341-342
+  ``"candidate %select{function|constructor}3%select{| template| %1}2 not viable"``.
+Class:
+  ``Integers``
+Description:
----------------
I don't think "Class:" makes sense here, since what this specifier consumes depends on its replacement string. "Class: varies" or similar might work, but maybe just drop it?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3526-3544
 def note_ovl_candidate : Note<"candidate "
     "%select{function|function|constructor|"
-    "function |function |constructor |"
     "is the implicit default constructor|"
     "is the implicit copy constructor|"
     "is the implicit move constructor|"
     "is the implicit copy assignment operator|"
     "is the implicit move assignment operator|"
----------------
Is there a reason this one wasn't changed to use `%sub`?


================
Comment at: test/TableGen/DiagnosticBase.inc:1
-// Define the diagnostic mappings.
-class DiagMapping;
-def MAP_IGNORE  : DiagMapping;
-def MAP_WARNING : DiagMapping;
-def MAP_ERROR   : DiagMapping;
-def MAP_FATAL   : DiagMapping;
+//===--- Diagnostic.td - C Language Family Diagnostic Handling ------------===//
+//
----------------
This is the wrong filename.


https://reviews.llvm.org/D46740