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.
(On the other hand I think this code should raise an error for using a
non-const global, but let's suppose you had a call to a constexpr()
function inside the enable_if expression. The same argument applies.)
Nick
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
