r199053 - Clarify warn_cxx98_compat_attribute diagnostic

[Aaron Ballman]

On Tue, Jan 14, 2014 at 9:10 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 14/01/2014 13:57, Aaron Ballman wrote:
>>
>> On Tue, Jan 14, 2014 at 3:25 AM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> On 14/01/2014 03:10, Richard Smith wrote:
>>>
>>> On Mon, Jan 13, 2014 at 6:37 PM, Aaron Ballman <aaron at aaronballman.com>
>>> wrote:
>>>>
>>>> On Mon, Jan 13, 2014 at 9:24 PM, Richard Smith <richard at metafoo.co.uk>
>>>> wrote:
>>>>>
>>>>> On Sun, Jan 12, 2014 at 11:14 AM, Aaron Ballman
>>>>> <aaron at aaronballman.com>
>>>>> wrote:
>>>>>>
>>>>>> After doing a bit more research and discussion off-list, I think
>>>>>> "generalized attribute" is acceptable.  So patch LGTM as-is.
>>>>>
>>>>>
>>>>> Really? I wouldn't expect someone seeing this diagnostic to understand
>>>>> that
>>>>> "generalized attribute" means C++11 attributes (it's a really weird
>>>>> term,
>>>>> since they're not a generalization of anything). This isn't an official
>>>>> name
>>>>> for them, and doesn't distinguish them from the other attribute
>>>>> syntaxes
>>>>> we
>>>>> support. Given that this is a diagnostic about compatibility with
>>>>> C++98,
>>>>> "C++11 attributes" seems like the clearest way of expressing this.
>>>>
>>>> As Alp had pointed out, we document the name as "generalized
>>>> attribute" in our feature support documentation,
>>>
>>>
>>> You're right, we did. I just fixed that.
>>>
>>>> and it's the original name of the feature.
>>>
>>>
>>> [citation needed]
>>>
>>> The proposal calls them "General Attributes for C++" (and all previous
>>> revisions of it did the same); the word "Generalized" seems to have been
>>> accidentally transferred from "Generalized constant expressions" in the
>>> GCC
>>> list, and we inherited that mistake when we sync'd our list with theirs
>>> in
>>> r142015. The paper and C++ standard both call them simply "attributes".
>>>
>>>> Also, a quick google search of "generalized
>>>> attribute" yielded more results than "C++11 attribute" did (not saying
>>>> this was particularly scientific). So that's why I gave the LGTM on
>>>> the term.
>>>
>>>
>>> Hah, it seems that lots of people copied our C++11 feature list and
>>> GCC's,
>>> picking up the wrong name =)
>>>
>>>
>>> It may be a typo, but the C++ community has clearly adopted the name
>>> "generalized attributes".
>>
>> I don't know if I'd say "clearly", considering my initial response to
>> the wording was "what's a generalized attribute?" ;-)
>>
>>> I think we should take it and run with it :-)
>>>
>>> Reasons to do so:
>>>
>>> "C++11 attributes" don't work as a feature name when bringing this to C11
>>> as
>>> an extension or enabling it in proposed next-generation OpenMP modes.
>>> It'd
>>> be bizarre to diagnose with "C++11 attribute ..." in C11.
>>
>> There's been some discussion on this, but nothing has actually been
>> proposed. So I wouldn't start rewording diagnostics based on this just
>> yet. ;-)
>>
>>> It feels old keeping the version of introduction in the name. We don't do
>>> this with other features that have been published, why introduce a
>>> special-case "C++11 attributes"?
>>
>> There is something to this argument. An extensive search through our
>> diagnostics show the usages of C++11 in the diagnostic text are
>> basically limited to two forms:
>>
>> XXX is a C++11 extension|feature
>> XXX does blah blah in C++11
>>
>> That suggests this warning should read:
>>
>> "attributes are a C++11 feature"
>
>
> I like this. In this context your suggestion seems clearer than both the
> previous diagnostic and my update to the wording in r199053.

Great!

>>
>> This has a natural benefit of being similar to the wording we would
>> use if we ever did bring C++11 attributes to other languages (except
>> we'd use 'extension' in place of 'feature').
>>
>> but...
>>
>>> Leaving the name as just "attributes" is ambiguous in this context
>>> because
>>> users have got used to "attributes" referring to the GNU form.
>>
>> The other parsing diagnostics refer to C++11 attributes as simply
>> "attributes", and refer to other attributes by their syntax, so this
>> change is not consistent with our other diagnostics.
>>
>> Given that C++11-style attributes are actually standardized, and GNU-
>> or MS-style attributes are not, what about turning this around a bit:
>>
>> C++11-style and syntax-agnostic attribute diagnostics get worded as
>> "attributes", __attribute__-style diagnostics get worded as "GNU
>> attributes" and __declspec-style diagnostics get worded as "__declspec
>> attributes".
>>
>> It's not wholly self-consistent (__declspec vs GNU), but I think
>> "__attribute__ attributes" looks horrible, and __declspec applies to
>> more than just Microsoft.
>>
>> Regardless, the fact that Chandler and Richard are both questioning
>> the new wording, I think it would make sense to roll this change back
>> (sorry for the false LGTM on my part!). If we're going to switch the
>> wording from the established convention within our own diagnostics, it
>> should have a bit more visible discussion and, more importantly, it
>> should be consistently applied across related diagnostics.
>
>
> Let's fix forward. The old diagnostic was obviously problematic but it looks
> like you have a reasonable alternative wording to go with in this specific
> context.

I'm fine with that. Also, to be clear: the "false LGTM" wasn't that
you did anything wrong, it was for my waffling after further
discussion and thought. ;-)

> That said, the naming problem for attributes isn't going to go away --  the
> desire has been expressed to use generalized attribute syntax in OpenMP and
> as a C11 extension, so we'll have to tackle this face-on sooner rather than
> later.

Yeah, that's the unfortunate reality of any overloaded term.
Thankfully, in most cases, the overload is only one or two uses. Here
it's three or four (so far!).

~Aaron