patch: new attribute enable_if

[Richard Smith]

On Mon, Sep 23, 2013 at 10:02 AM, Aaron Ballman <aaron at aaronballman.com>wrote:

> 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


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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130923/05d5c59e/attachment.html>