r206186 - Properly diagnose standard C++ attributes which have optional argument lists when the arguments are elided. eg)
Aaron Ballman
aaron at aaronballman.com
Mon Apr 14 18:24:32 PDT 2014
On Mon, Apr 14, 2014 at 8:43 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> All good comments -- I've applied everything but the fixit in r206229
> (I put in a fixme for the fixit).
>
> Thanks!
>
> ~Aaron
>
> On Mon, Apr 14, 2014 at 8:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Mon, Apr 14, 2014 at 9:03 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> Author: aaronballman
>>> Date: Mon Apr 14 11:03:22 2014
>>> New Revision: 206186
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=206186&view=rev
>>> Log:
>>> Properly diagnose standard C++ attributes which have optional argument
>>> lists when the arguments are elided. eg)
>>>
>>> [[deprecated()]] // error
>>> [[deprecated]] // OK
>>> [[deprecated("")]] // OK
>>> [[gnu::deprecated()]] // OK
>>>
>>> Modified:
>>> cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>> cfe/trunk/include/clang/Parse/Parser.h
>>> cfe/trunk/lib/Parse/ParseDecl.cpp
>>> cfe/trunk/lib/Parse/ParseDeclCXX.cpp
>>> cfe/trunk/test/Parser/attributes.c
>>> cfe/trunk/test/Parser/cxx0x-attributes.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Apr 14
>>> 11:03:22 2014
>>> @@ -526,7 +526,9 @@ def warn_cxx98_compat_attribute : Warnin
>>> "C++11 attribute syntax is incompatible with C++98">,
>>> InGroup<CXX98Compat>, DefaultIgnore;
>>> def err_cxx11_attribute_forbids_arguments : Error<
>>> - "attribute '%0' cannot have an argument list">;
>>> + "attribute %0 cannot have an argument list">;
>>> +def err_attribute_requires_arguements : Error<
>>
>>
>> Typo 'arguements'
>>
>>>
>>> + "attribute %0 requires a nonempty argument list">;
>>
>>
>> It'd be nicer to say something more like "parentheses must be omitted if %0
>> attribute's argument list is empty". The user probably didn't want to give
>> an argument here, and we shouldn't suggest that one is necessary.
>>
>>>
>>> def err_cxx11_attribute_forbids_ellipsis : Error<
>>> "attribute '%0' cannot be used as an attribute pack">;
>>> def err_cxx11_attribute_repeated : Error<
>>>
>>> Modified: cfe/trunk/include/clang/Parse/Parser.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Parse/Parser.h (original)
>>> +++ cfe/trunk/include/clang/Parse/Parser.h Mon Apr 14 11:03:22 2014
>>> @@ -2012,13 +2012,13 @@ private:
>>>
>>> /// \brief Parses syntax-generic attribute arguments for attributes
>>> which are
>>> /// known to the implementation, and adds them to the given
>>> ParsedAttributes
>>> - /// list with the given attribute syntax.
>>> - void ParseAttributeArgsCommon(IdentifierInfo *AttrName,
>>> - SourceLocation AttrNameLoc,
>>> - ParsedAttributes &Attrs, SourceLocation
>>> *EndLoc,
>>> - IdentifierInfo *ScopeName,
>>> - SourceLocation ScopeLoc,
>>> - AttributeList::Syntax Syntax);
>>> + /// list with the given attribute syntax. Returns the number of
>>> arguments
>>> + /// parsed for the attribute.
>>> + unsigned
>>> + ParseAttributeArgsCommon(IdentifierInfo *AttrName, SourceLocation
>>> AttrNameLoc,
>>> + ParsedAttributes &Attrs, SourceLocation
>>> *EndLoc,
>>> + IdentifierInfo *ScopeName, SourceLocation
>>> ScopeLoc,
>>> + AttributeList::Syntax Syntax);
>>>
>>> void MaybeParseGNUAttributes(Declarator &D,
>>> LateParsedAttrList *LateAttrs = 0) {
>>>
>>> Modified: cfe/trunk/lib/Parse/ParseDecl.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseDecl.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseDecl.cpp Mon Apr 14 11:03:22 2014
>>> @@ -261,7 +261,7 @@ void Parser::ParseAttributeWithTypeArg(I
>>> 0, AttrNameLoc, 0, 0, AttributeList::AS_GNU);
>>> }
>>>
>>> -void Parser::ParseAttributeArgsCommon(
>>> +unsigned Parser::ParseAttributeArgsCommon(
>>> IdentifierInfo *AttrName, SourceLocation AttrNameLoc,
>>> ParsedAttributes &Attrs, SourceLocation *EndLoc, IdentifierInfo
>>> *ScopeName,
>>> SourceLocation ScopeLoc, AttributeList::Syntax Syntax) {
>>> @@ -302,7 +302,7 @@ void Parser::ParseAttributeArgsCommon(
>>> ExprResult ArgExpr(ParseAssignmentExpression());
>>> if (ArgExpr.isInvalid()) {
>>> SkipUntil(tok::r_paren, StopAtSemi);
>>> - return;
>>> + return 0;
>>> }
>>> ArgExprs.push_back(ArgExpr.release());
>>> // Eat the comma, move to the next argument
>>> @@ -318,6 +318,8 @@ void Parser::ParseAttributeArgsCommon(
>>>
>>> if (EndLoc)
>>> *EndLoc = RParen;
>>> +
>>> + return static_cast<unsigned>(ArgExprs.size());
>>> }
>>>
>>> /// Parse the arguments to a parameterized GNU attribute or
>>>
>>> Modified: cfe/trunk/lib/Parse/ParseDeclCXX.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDeclCXX.cpp?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseDeclCXX.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseDeclCXX.cpp Mon Apr 14 11:03:22 2014
>>> @@ -3194,6 +3194,7 @@ static bool IsBuiltInOrStandardCXX11Attr
>>> switch (AttributeList::getKind(AttrName, ScopeName,
>>> AttributeList::AS_CXX11)) {
>>> case AttributeList::AT_CarriesDependency:
>>> + case AttributeList::AT_Deprecated:
>>> case AttributeList::AT_FallThrough:
>>> case AttributeList::AT_CXX11NoReturn: {
>>> return true;
>>> @@ -3225,6 +3226,7 @@ bool Parser::ParseCXX11AttributeArgs(Ide
>>> IdentifierInfo *ScopeName,
>>> SourceLocation ScopeLoc) {
>>> assert(Tok.is(tok::l_paren) && "Not a C++11 attribute argument list");
>>> + SourceLocation LParenLoc = Tok.getLocation();
>>>
>>> // If the attribute isn't known, we will not attempt to parse any
>>> // arguments.
>>> @@ -3241,9 +3243,32 @@ bool Parser::ParseCXX11AttributeArgs(Ide
>>> // behaviors.
>>> ParseGNUAttributeArgs(AttrName, AttrNameLoc, Attrs, EndLoc,
>>> ScopeName,
>>> ScopeLoc, AttributeList::AS_CXX11, 0);
>>> - else
>>> - ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, EndLoc,
>>> ScopeName,
>>> - ScopeLoc, AttributeList::AS_CXX11);
>>> + else {
>>> + unsigned NumArgs =
>>> + ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, EndLoc,
>>> + ScopeName, ScopeLoc,
>>> AttributeList::AS_CXX11);
>>> +
>>> + const AttributeList *Attr = Attrs.getList();
>>> + if (Attr && IsBuiltInOrStandardCXX11Attribute(AttrName, ScopeName)) {
>>> + // If the attribute is a standard or built-in attribute and we are
>>> + // parsing an argument list, we need to determine whether this
>>> attribute
>>> + // was allowed to have an argument list (such as [[deprecated]]),
>>> and how
>>> + // many arguments were parsed (so we can diagnose on
>>> [[deprecated()]]).
>>> + if (Attr->getMaxArgs() && !NumArgs) {
>>> + // The attribute was allowed to have arguments, but none were
>>> provided
>>> + // even though the attribute parsed successfully. This is an
>>> error.
>>> + Diag(LParenLoc, diag::err_attribute_requires_arguements) <<
>>> AttrName;
>>> + return false;
>>> + } else if (!Attr->getMaxArgs()) {
>>> + // The attribute parsed successfully, but was not allowed to have
>>> any
>>> + // arguments. It doesn't matter whether any were provided -- the
>>> + // presence of the argument list (even if empty) is diagnosed.
>>> + Diag(LParenLoc, diag::err_cxx11_attribute_forbids_arguments)
>>> + << AttrName;
>>> + return false;
>>> + }
>>
>>
>> I think it'd be better to use the "remove the ()" diagnostic whenever
>> NumArgs is 0, and use the "forbids arguments" diagnostic whenever NumArgs is
>> nonzero and getMaxArgs() is 0. The "remove the ()" diagnostic should
>> probably also get a fixit removing the parens.
So it turns out that the logic doesn't quite work out well in terms of
the new diagnostic wording.
[[noreturn()]] void c();
This code used to fire: attribute 'noreturn' cannot have an argument list
Now it fires: parentheses must be omitted if 'noreturn' attribute's
argument list is empty
That implies noreturn's argument list could be nonempty, which isn't
correct. I'm partially reverting this part of the change while I think
on it a bit further (this will unbreak build bots, even though the
tests inexplicably passed for me locally).
>>
>>> + }
>>> + }
>>> return true;
>>> }
>>>
>>> @@ -3324,14 +3349,9 @@ void Parser::ParseCXX11AttributeSpecifie
>>> << AttrName << SourceRange(SeenAttrs[AttrName]);
>>>
>>> // Parse attribute arguments
>>> - if (Tok.is(tok::l_paren)) {
>>> - if (StandardAttr)
>>> - Diag(Tok.getLocation(),
>>> diag::err_cxx11_attribute_forbids_arguments)
>>> - << AttrName->getName();
>>> -
>>> + if (Tok.is(tok::l_paren))
>>> AttrParsed = ParseCXX11AttributeArgs(AttrName, AttrLoc, attrs,
>>> endLoc,
>>> ScopeName, ScopeLoc);
>>> - }
>>>
>>> if (!AttrParsed)
>>> attrs.addNew(AttrName,
>>>
>>> Modified: cfe/trunk/test/Parser/attributes.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/attributes.c?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/Parser/attributes.c (original)
>>> +++ cfe/trunk/test/Parser/attributes.c Mon Apr 14 11:03:22 2014
>>> @@ -94,5 +94,4 @@ void testFundef5() __attribute__(()) { }
>>>
>>> __attribute__((pure)) int testFundef6(int a) { return a; }
>>>
>>> -
>>> -
>>> +void deprecatedTestFun(void) __attribute__((deprecated()));
>>>
>>> Modified: cfe/trunk/test/Parser/cxx0x-attributes.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx0x-attributes.cpp?rev=206186&r1=206185&r2=206186&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/Parser/cxx0x-attributes.cpp (original)
>>> +++ cfe/trunk/test/Parser/cxx0x-attributes.cpp Mon Apr 14 11:03:22 2014
>>> @@ -322,3 +322,10 @@ namespace GccASan {
>>> [[gnu::no_address_safety_analysis]] void f3();
>>> [[gnu::no_sanitize_address]] void f4();
>>> }
>>> +
>>> +namespace {
>>> + [[deprecated]] void bar();
>>> + [[deprecated("hello")]] void baz();
>>> + [[deprecated()]] void foo(); // expected-error {{attribute 'deprecated'
>>> requires a nonempty argument list}}
>>> + [[gnu::deprecated()]] void quux();
>>> +}
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
~Aaron
More information about the cfe-commits
mailing list