patch: new attribute enable_if

[Nick Lewycky]

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.

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

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

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enable-if-2.patch
Type: text/x-patch
Size: 32497 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130922/52520b5d/attachment.bin>