r198605 - Don't use magic constants in the digraph diagnostic

[Alp Toker]

On 07/01/2014 02:19, Jordan Rose wrote:
> On Jan 6, 2014, at 18:14, Alp Toker <alp at nuanti.com> wrote:
>
>> On 06/01/2014 19:11, Richard Smith wrote:
>>> On Mon Jan 06 2014 at 11:01:02 AM, Alp Toker <alp at nuanti.com <mailto:alp at nuanti.com>> wrote:
>>>
>>>
>>>     On 06/01/2014 18:29, Richard Smith wrote:
>>>     > On Mon Jan 06 2014 at 10:06:25 AM, Alp Toker <alp at nuanti.com
>>>     <mailto:alp at nuanti.com>
>>>     > <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
>>>     >
>>>     >
>>>     >     On 06/01/2014 17:34, Jordan Rose wrote:
>>>     >     > This changes the message for templates in a way that is less
>>>     >     informative.
>>>     >
>>>     >     No change in output. Do you mean the string in the TableGen file
>>>     >     is now
>>>     >     less informative?
>>>     >
>>>     >     > In other cases where we use tokens as diagnostic
>>>     arguments, the
>>>     >     message makes sense whether or not the token text is quoted.
>>>     Here,
>>>     >     that's not true: the problem isn't about the literal word
>>>     >     "template" but about a particular template (like "vector").
>>>     Using
>>>     >     tok::annot_template_id as if it were a real token seems wrong
>>>     >
>>>     >     That was the reason I changed the "magic token kind" in use from
>>>     >     tok::kw_template to tok::annot_template_id. tok::kw_template
>>>     >     relates to
>>>     >     "template" whereas tok::annot_template_id is a synonym for
>>>     "template
>>>     >     name" so this appears a better choice to use as the placeholder.
>>>     >
>>>     >
>>>     > It's a synonym for 'template id' (a template name followed by
>>>     template
>>>     > arguments), not for 'template name'. Using it here is inappropriate.
>>>
>>>     Diagnostics seem to call it that way, with 'template-id' not appearing
>>>     rarely:
>>>
>>>     def err_template_id_not_a_type : Error<
>>>        "template name refers to non-type template %0">;
>>>
>>>
>>> This looks clear and technically correct to me. We're diagnosing that the template-id X<Y> is not a type by saying that X is a non-type template name.
>>>
>>>     I agree it's not fully appropriate, but it's more appropriate than the
>>>     previous tok::kw_template given that there isn't a "template"
>>>     keyword in
>>>     sight:
>>>
>>>     D<::F> A2; // expected-error{{found '<::' after a template name which
>>>     forms the digraph}}
>>>
>>>     Can you think of a better alternative to either?
>>>
>>>
>>> Jordan's suggestion of using an enum seems fine to me. (As Aaron Ballman and I have previously discussed on-list, we should also have a mechanism to generate these enums from the .td files to avoid the magic numbers entirely.)
>> Even with an enum there's still going to have to be a switch somewhere, and what I've really been looking for is boilerplate-free way to share code construct descriptions with the parser, sema and code completion.
>>
>> Agree that this change might not be the solution (especially annot_type_id) but the previous code was at least as dubious with its kw_template so I'm not keen on bringing that back to life either.
>>
>> So if it's OK with Jordan I'd like to mull over this for a few days to see if we can move it forward in a satisfactory way first. Will examine those threads again too.
>>
>> Alp.
> The previous code was silly, but it was just using kw_template as a switch key. If you want to use tok::identifier there instead, that's fine, but I'd be happier if the structure were put back the way it originally was for now.

Backed out, including suggested cleanups.

r198666.

Better the beast we know, eh :-)

Alp.


>
> Jordan

-- 
http://www.nuanti.com
the browser experts