aaron.ballman added a comment.
In https://reviews.llvm.org/D37436#867287, @rsmith wrote:
> If this is just supposed to be an experiment to get feedback on the feature,
> then I don't think we should be treating it as a different attribute syntax
> at all. Rather, I think we
> just want to permit C++11 attributes to be parsed in other language modes.
> If/when this becomes part of some future C working draft, I think that's the
> time to have a
> separate attribute syntax with a distinct set of valid unqualified attribute
> names.
I do not think that's the correct approach. These are not C++ attributes (for
instance, no `using` insanity is allowed, `::` is a new lexing token in C,
etc). Also, I don't think it's a good idea to enable all C++11-style attributes
in C mode without giving each attribute some appropriate thought (what does
`abi_tag` *do* in C mode? What happens with _Noreturn vs [[noreturn]] etc).
Also, I'm not comfortable adding a bunch of vendor-specific `gnu::` attributes
that GCC does not implement in C yet.
The goal of this is to get a working implementation of the attribute syntax
that WG14 has shown strong support for including in C2x (F: 16, N: 3, A: 1)
with this syntax (F: 12, N: 3, A: 4). The experimental part where I need
feedback is whether the paper needs to change because of implementation
experience. So far, that's not been required. The more we drift from what's
proposed in N2137, the less useful the implementation experience will be (esp
if we basically turn this into a simple "enable C++ attributes everywhere"
flag). I can understand not adding a C2x language mode due to the lack of a
working draft, but that lack should not be what drives internal implementation
details for the feature -- I think the separation of concerns in this patch is
valuable.
================
Comment at: include/clang/Basic/LangOptions.def:140
+LANGOPT(CAttributes , 1, 0, "'[[]]' attributes extension to C")
+
----------------
rsmith wrote:
> Can we give this a more general name, then enable it under `CPlusPlus` too?
> That's what we do for other similar features.
I'm not keen on naming it 'Attributes', but the other names are worse
(`GeneralAttributes`, `StandardAttributes`). Do you have a suggestion?
================
Comment at: include/clang/Driver/Options.td:607
+def fc_attributes : Flag<["-"], "fc-attributes">, Group<f_Group>,
+ Flags<[DriverOption, CC1Option]>, HelpText<"Enable '[[]]' attributes in C">;
----------------
rsmith wrote:
> Hmm. On reflection, if we're going to have a flag to enable C++11 attributes
> in other language modes, it should probably work in C++98 mode too, so
> calling this `-fc-attributes` is probably not the best idea.
> `-fc++11-attributes` might make sense, though.
I can't think of a reason why you'd want to control C and C++ attributes
separately, so I think it makes sense to add a more general name for this.
I'm definitely not keen on that flag name. I wouldn't be opposed to
`-fattributes`, but that may lead to confusion about other vendor-specific
attributes (which we could always decide to use flags like
`-fdeclspec-attributes` etc).
What should happen if a user compiles with `-std=c++11
-fno-<whatever>-attributes`? Do we want to support such a construct?
================
Comment at: lib/Parse/ParseDecl.cpp:4219
+
+ // Attributes are prohibited in this location in C2x (and forward
+ // declarations are prohibited in C++).
----------------
rsmith wrote:
> I don't think we can reasonably say what C2x will do. Also, doesn't this
> reject valid code such as:
> ```
> struct __attribute__((deprecated)) Foo;
> ```
> ?
Sorry for the lack of context in the patch (TortoiseSVN doesn't make this
easy). This has to do with enum specifiers, not struct or unions -- it will
reject `enum __attribute__((deprecated)) Foo;` as not allowing an attribute
list *and* as being a forward reference in C++ mode, but accepts in C mode.
================
Comment at: lib/Parse/ParseDeclCXX.cpp:3939-3940
SourceLocation *endLoc) {
- if (Tok.is(tok::kw_alignas)) {
+ bool CXXAttr = getLangOpts().CPlusPlus11;
+ if (CXXAttr && Tok.is(tok::kw_alignas)) {
Diag(Tok.getLocation(), diag::warn_cxx98_compat_alignas);
----------------
rsmith wrote:
> This change is unnecessary. `kw_alignas` is not produced by the lexer in
> modes where we should not parse it.
Ah, good to know. I'll remove.
================
Comment at: lib/Parse/ParseDeclCXX.cpp:3956
IdentifierInfo *CommonScopeName = nullptr;
- if (Tok.is(tok::kw_using)) {
+ if (CXXAttr && Tok.is(tok::kw_using)) {
Diag(Tok.getLocation(), getLangOpts().CPlusPlus1z
----------------
rsmith wrote:
> Likewise.
I'll add a test for this to ensure we don't accidentally allow it in C mode.
https://reviews.llvm.org/D37436
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
