patch: new attribute enable_if

[Aaron Ballman]

On Wed, Sep 25, 2013 at 1:46 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Aaron Ballman wrote:
>>
>> On Sun, Sep 22, 2013 at 5:57 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>>
>>> Aaron Ballman wrote:
>>>>
>>>>
>>>> Most of my comments are related to the attribute itself.  I've got to
>>>> run, but can take another look tomorrow as well.
>>>
>>>
>>>
>>> Thanks! Unless I replied to it here, I've done what you suggested.
>>> Updated
>>> patch attached!
>>>
>>>
>>>>> +def EnableIf : InheritableAttr {
>>>>> +  let Spellings = [GNU<"enable_if">];
>>>>
>>>>
>>>>
>>>> Is this really a GNU attribute?  If not, perhaps since this is related
>>>> to overloading, this could have a C++11 style clang:: attribute.
>>>> (Reading further, I see tests for C as well, so perhaps not.)
>>>
>>>
>>>
>>> As much as __attribute__((overloadable)) is. It's intended to be used
>>> with
>>> the GNU syntax for attributes, but isn't implemented by GCC.
>>
>>
>> Okay
>>
>>>
>>>
>>>>> +  if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(FDecl)) {
>>>>> +    if (FD->hasAttr<EnableIfAttr>()) {
>>>>> +      ArrayRef<Expr *>   NonConstArgs =
>>>>> +        llvm::makeArrayRef<Expr*>(const_cast<Expr**>(Args.data()),
>>>>> Args.size());
>>>>
>>>>
>>>>
>>>> This const_cast makes me a bit nervous; is there a way to avoid it?
>>>
>>>
>>>
>>> Not really, I can move it around a bit or I can remove consts all over
>>> the
>>> place. I tried sinking it into the callee, but it so happens that adding
>>> const requires just as nasty a statement.
>>>
>>>>> Index: lib/Sema/SemaDeclAttr.cpp
>>>>> ===================================================================
>>>>> --- lib/Sema/SemaDeclAttr.cpp (revision 191171)
>>>>> +++ lib/Sema/SemaDeclAttr.cpp (working copy)
>>>>> @@ -982,6 +982,33 @@
>>>>>
>>>>> Attr.getAttributeSpellingListIndex()));
>>>>>    }
>>>>>
>>>>> +static void handleEnableIfAttr(Sema&S, Decl *D, const
>>>>> AttributeList&Attr) {
>>>>> +  if (!isa<FunctionDecl>(D)&&   !isa<FunctionTemplateDecl>(D)) {
>>>>>
>>>>> +    S.Diag(Attr.getLoc(), diag::err_attribute_enable_if_not_function);
>>>>> +    return;
>>>>> +  }
>>>>
>>>>
>>>>
>>>> Does this apply to function-like things as well (such as function
>>>> pointers, etc)?
>>>
>>>
>>>
>>> No.
>>>
>>>
>>>>> +
>>>>> +  if (!checkAttributeNumArgs(S, Attr, 2))
>>>>> +    return;
>>>>> +
>>>>> +  Expr *Cond = Attr.getArgAsExpr(0);
>>>>> +  Expr *Message = Attr.getArgAsExpr(1);
>>>>
>>>>
>>>>
>>>> Attributes can take unresolved identifiers as well as expressions (for
>>>> the first argument position), so you should be prepared to handle a
>>>> case like that.
>>>
>>>
>>>
>>> Sorry, are you saying that __attribute__((enable_if(id, ...))) could fail
>>> to
>>> return an Expr? I tried that syntax and it works fine (and added a test),
>>> but I'm wondering if there's another syntax you had in mind which I
>>> haven't
>>> thought of?
>>
>>
>> Actually, re-reading the code, I think you're fine because you're
>> using an ExprArgument as the first parameter to the attribute.  So you
>> will always get resolved expressions.  Sorry for the confusion!
>>
>>>
>>>
>>>> Missing test cases for everything in SemaDeclAttr.cpp.  Be sure to hit
>>>> edge cases as well (such as unresolved identifier as the expression).
>>>
>>>
>>>
>>> Thanks! I added a bunch, let me know if there's anything I missed.
>>
>>
>>> +def EnableIf : InheritableAttr {
>>> +  let Spellings = [GNU<"enable_if">];
>>> +  let Subjects = [Function];
>>> +  let Args = [ExprArgument<"Cond">, StringArgument<"Message">];
>>> +  let TemplateDependent = 1;
>>> +}
>>
>>
>> Subjects should also include FunctionTemplate since that's what you're
>> checking in SemaDeclAttr.cpp
>
>
> Removed FunctionTemplate as pointed out by Richard.
>
>>> +static void handleEnableIfAttr(Sema&S, Decl *D, const
>>> AttributeList&Attr) {
>>> +  if (!isa<FunctionDecl>(D)&&  !isa<FunctionTemplateDecl>(D)) {
>>> +    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
>>> +<<  Attr.getName()<<  ExpectedFunctionOrMethod;
>>
>>
>> This should be ExpectedFunction (unless you mean to support Obj-C
>> methods, in which case you'd need to update the Subjects and isa
>> check).
>
>
> Indeed, I was thinking C++ methods not ObjC methods. Fixed!
>
>
>>> +    return;
>>> +  }
>>> +
>>> +  if (!checkAttributeNumArgs(S, Attr, 2))
>>> +    return;
>>> +
>>> +  Expr *Cond = Attr.getArgAsExpr(0);
>>> +  ExprResult Converted = S.PerformContextuallyConvertToBool(Cond);
>>> +  if (Converted.isInvalid())
>>> +    return;
>>> +
>>> +  StringRef Msg;
>>> +  if (!checkStringLiteralArgument(S, Msg, Attr, 1))
>>> +    return;
>>> +
>>> +  D->addAttr(::new (S.Context) EnableIfAttr(Attr.getRange(), S.Context,
>>> +                                            Converted.take(), Msg));
>>
>>
>> You should also pass in the spelling list index to the constructor.
>
>
> Fixed! Thanks!
>
>
>>> +}
>>
>>
>> One random question -- what happens if you do the following:
>>
>> int n __attribute__((deprecated));
>> void f(int x) __attribute__((enable_if(n == 0, ""));
>>
>> Will you get a deprecated message with or without calling f?
>
>
> Without calling f:
>
> aaron.c:2:40: warning: 'n' is deprecated [-Wdeprecated-declarations]
> void f(int x) __attribute__((enable_if(n == 0, "")));
>                                        ^
> aaron.c:1:5: note: 'n' declared here
> int n __attribute__((deprecated));
>     ^
> 1 warning generated.
>
> which I think is the correct time and place to diagnose it. If you wanted a
> deprecated warning to depend on which overload is selected, put it on the
> same function that has the enable_if.

I think that is reasonable as well.  Great!

~Aaron