<div dir="ltr">On Mon, Sep 23, 2013 at 10:02 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Sun, Sep 22, 2013 at 5:57 PM, Nick Lewycky <<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>> wrote:<br>
> Aaron Ballman wrote:<br>
>><br>
>> Most of my comments are related to the attribute itself. I've got to<br>
>> run, but can take another look tomorrow as well.<br>
><br>
><br>
> Thanks! Unless I replied to it here, I've done what you suggested. Updated<br>
> patch attached!<br>
><br>
><br>
>>> +def EnableIf : InheritableAttr {<br>
>>> + let Spellings = [GNU<"enable_if">];<br>
>><br>
>><br>
>> Is this really a GNU attribute? If not, perhaps since this is related<br>
>> to overloading, this could have a C++11 style clang:: attribute.<br>
>> (Reading further, I see tests for C as well, so perhaps not.)<br>
><br>
><br>
> As much as __attribute__((overloadable)) is. It's intended to be used with<br>
> the GNU syntax for attributes, but isn't implemented by GCC.<br>
<br>
</div>Okay<br>
<div><div class="h5"><br>
><br>
><br>
>>> + if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(FDecl)) {<br>
>>> + if (FD->hasAttr<EnableIfAttr>()) {<br>
>>> + ArrayRef<Expr *> NonConstArgs =<br>
>>> + llvm::makeArrayRef<Expr*>(const_cast<Expr**>(Args.data()),<br>
>>> Args.size());<br>
>><br>
>><br>
>> This const_cast makes me a bit nervous; is there a way to avoid it?<br>
><br>
><br>
> Not really, I can move it around a bit or I can remove consts all over the<br>
> place. I tried sinking it into the callee, but it so happens that adding<br>
> const requires just as nasty a statement.<br>
><br>
>>> Index: lib/Sema/SemaDeclAttr.cpp<br>
>>> ===================================================================<br>
>>> --- lib/Sema/SemaDeclAttr.cpp (revision 191171)<br>
>>> +++ lib/Sema/SemaDeclAttr.cpp (working copy)<br>
>>> @@ -982,6 +982,33 @@<br>
>>> Attr.getAttributeSpellingListIndex()));<br>
>>> }<br>
>>><br>
>>> +static void handleEnableIfAttr(Sema&S, Decl *D, const<br>
>>> AttributeList&Attr) {<br>
>>> + if (!isa<FunctionDecl>(D)&& !isa<FunctionTemplateDecl>(D)) {<br>
>>><br>
>>> + S.Diag(Attr.getLoc(), diag::err_attribute_enable_if_not_function);<br>
>>> + return;<br>
>>> + }<br>
>><br>
>><br>
>> Does this apply to function-like things as well (such as function<br>
>> pointers, etc)?<br>
><br>
><br>
> No.<br>
><br>
><br>
>>> +<br>
>>> + if (!checkAttributeNumArgs(S, Attr, 2))<br>
>>> + return;<br>
>>> +<br>
>>> + Expr *Cond = Attr.getArgAsExpr(0);<br>
>>> + Expr *Message = Attr.getArgAsExpr(1);<br>
>><br>
>><br>
>> Attributes can take unresolved identifiers as well as expressions (for<br>
>> the first argument position), so you should be prepared to handle a<br>
>> case like that.<br>
><br>
><br>
> Sorry, are you saying that __attribute__((enable_if(id, ...))) could fail to<br>
> return an Expr? I tried that syntax and it works fine (and added a test),<br>
> but I'm wondering if there's another syntax you had in mind which I haven't<br>
> thought of?<br>
<br>
</div></div>Actually, re-reading the code, I think you're fine because you're<br>
using an ExprArgument as the first parameter to the attribute. So you<br>
will always get resolved expressions. Sorry for the confusion!<br>
<div class="im"><br>
><br>
><br>
>> Missing test cases for everything in SemaDeclAttr.cpp. Be sure to hit<br>
>> edge cases as well (such as unresolved identifier as the expression).<br>
><br>
><br>
> Thanks! I added a bunch, let me know if there's anything I missed.<br>
<br>
</div><div class="im">> +def EnableIf : InheritableAttr {<br>
> + let Spellings = [GNU<"enable_if">];<br>
</div>> + let Subjects = [Function];<br>
<div class="im">> + let Args = [ExprArgument<"Cond">, StringArgument<"Message">];<br>
> + let TemplateDependent = 1;<br>
</div>> +}<br>
<br>
Subjects should also include FunctionTemplate since that's what you're<br>
checking in SemaDeclAttr.cpp</blockquote><div><br></div><div>I suspect the change should be to remove FunctionTemplateDecl there, not add it here: this should apply to the FunctionDecl within a FunctionTemplateDecl, not to the template itself, right?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +static void handleEnableIfAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
</div>> + if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {<br>
> + S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)<br>
> + << Attr.getName() << ExpectedFunctionOrMethod;<br>
<br>
This should be ExpectedFunction (unless you mean to support Obj-C<br>
methods, in which case you'd need to update the Subjects and isa<br>
check).<br>
<br>
> + return;<br>
> + }<br>
<div class="im">> +<br>
> + if (!checkAttributeNumArgs(S, Attr, 2))<br>
> + return;<br>
> +<br>
> + Expr *Cond = Attr.getArgAsExpr(0);<br>
</div><div class="im">> + ExprResult Converted = S.PerformContextuallyConvertToBool(Cond);<br>
> + if (Converted.isInvalid())<br>
> + return;<br>
> +<br>
</div>> + StringRef Msg;<br>
> + if (!checkStringLiteralArgument(S, Msg, Attr, 1))<br>
<div class="im">> + return;<br>
> +<br>
> + D->addAttr(::new (S.Context) EnableIfAttr(Attr.getRange(), S.Context,<br>
</div>> + Converted.take(), Msg));<br>
<br>
You should also pass in the spelling list index to the constructor.<br>
<br>
> +}<br>
<br>
One random question -- what happens if you do the following:<br>
<br>
int n __attribute__((deprecated));<br>
void f(int x) __attribute__((enable_if(n == 0, ""));<br>
<br>
Will you get a deprecated message with or without calling f?<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>