On Mon, Sep 23, 2013 at 10:02 AM, Aaron Ballman <[email protected]>wrote:
> On Sun, Sep 22, 2013 at 5:57 PM, Nick Lewycky <[email protected]> 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 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? > +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 > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
