On Wed, Sep 25, 2013 at 1:46 AM, Nick Lewycky <[email protected]> wrote: > Aaron Ballman 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 > > > 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.
I think that is reasonable as well. Great! ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
