patch: new attribute enable_if

[Aaron Ballman]

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

> +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).

> +    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.

> +}

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?

Thanks!

~Aaron