r199053 - Clarify warn_cxx98_compat_attribute diagnostic

Tue Jan 14 14:05:41 PST 2014

On Tue Jan 14 2014 at 1:33:44 PM, Sean Silva <silvas at purdue.edu> wrote:

On Tue, Jan 14, 2014 at 4:29 PM, Sean Silva <silvas at purdue.edu> wrote:

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

This is the diagnostic form we use when a C++11 feature is used in C++98,
not the form we use for -Wc++98-compat (when the feature is used in C++11).
For the latter, we almost exclusively use a "<blah> is/are incompatible
with C++98" wording. (There are 6 exceptions in 76 diagnostics: 5x"<blah>
is/would be/might be <blah> in C++98", 1x"C++98 requires <blah>")

So for consistency, this should be phrased as "<blah> attributes are
incompatible with C++98", fsvo <blah>.

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.

Actually, some people use the term "brackets" to collectively refer to ( [
{ ("round brackets", "square brackets", "curly brackets", respectively).
Better to just say "`[[` attributes".

Well:
1) the context is an "incompatible with C++98" diagnostic
2) we really don't need to worry about theoretical future C17 or OpenMP
constructs now; we can easily change our diagnostics if we ever support
those things
3) we have the '[[' in the snippet
4) a %select is *better* than a single fixed string if it makes the
diagnostic clearer

I'm fine with "'[[...]]' attributes" or similar, although it seems a little
redundant given (3). "C++ attributes" or "C++11 attributes" work for me.

-- Sean Silva

-- 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/e7749537/attachment.html>