patch: new attribute enable_if

[Nick Lewycky]

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.

(On the other hand I think this code should raise an error for using a 
non-const global, but let's suppose you had a call to a constexpr() 
function inside the enable_if expression. The same argument applies.)

Nick