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

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

rsmith added a comment.

Thank you, I've been wanting this for ages but never got around to it :)

(I'd been thinking about doing this by adding custom enum -> text mappings, but I think your approach is at least somewhat more general.)



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3626-3634
+    "%select{function|function|constructor|"
+    "function |function |constructor |"
     "constructor (the implicit default constructor)|"
     "constructor (the implicit copy constructor)|"
     "constructor (the implicit move constructor)|"
     "function (the implicit copy assignment operator)|"
     "function (the implicit move assignment operator)|"
----------------
Why do some of these have trailing spaces and others not? That looks wrong to me.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3648
 def note_ovl_candidate_deleted : Note<
-    "candidate %select{function|function|constructor|"
-    "function |function |constructor |"
-    "constructor (the implicit default constructor)|"
-    "constructor (the implicit copy constructor)|"
-    "constructor (the implicit move constructor)|"
-    "function (the implicit copy assignment operator)|"
-    "function (the implicit move assignment operator)|"
-    "inherited constructor|"
-    "inherited constructor }0%1 has been "
+    "candidate %sub{select_ovl_template_kind}0%1 has been "
     "%select{explicitly made unavailable|explicitly deleted|"
----------------
... and this is the reason. `%1` is sometimes empty and sometimes non-empty, and we're "cunningly" passing that information through `%0`. Yuck.

It looks like this particular diagnostic refactoring can't work quite like this if we want it to be whitespace-correct (which I think we do), because some uses of your factored-out text also include a name and others don't. Perhaps adding another select on `%0` that either adds a space or not would work, or two separate substitutions for the "include name" and "don't include name" cases?

More generally I wonder if we actually want to renumber `%i`'s through a `%sub`, so that we could define the substitution as

    TextSubstitution<
       "%select{function|function|constructor|function %1|function %1|constructor %1|...}0">

and then use it as, eg

    "candidate %sub{select_ovl_candidate_kind_with_name}5,6 not viable"

and have it act as if we wrote

    "candidate %select{function|function|constructor|function %6|function %6|constructor %6|...}5 not viable">

I expect we'll need that pretty much as soon as we have a substitution that takes more than one parameter, and I think we want a multi-parameter substitution for this refactoring :)


https://reviews.llvm.org/D46740