r199053 - Clarify warn_cxx98_compat_attribute diagnostic

[Sean Silva]

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.
>
>
>
>> 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.
>
> 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.
>
> It'll be sad if we end up with "%select{C++11|C11|OpenMP} attributes" a
> year down the line because decisions were made for the wrong reason today
> and we have to make changes with a view to the entire parser, not just
> individual standards as they stand today.


WTF just say "double-bracket attributes" and be done with it.

-- Sean Silva


>
>
> Alp.
>
>
>
>
>
>> ~Aaron
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140114/89473008/attachment.html>