patch: new attribute enable_if
Richard Smith
richard at metafoo.co.uk
Mon Sep 23 19:59:57 PDT 2013
Hi Nick,
This looks really promising. Do you yet know whether this is enough to
implement *all* the functionality of -DFORTIFY_SOURCE?
One big thing missing here is documentation. That should serve to both
motivate the feature (and explain why it's sufficiently valuable for us to
have and maintain this extension), and to describe how it's intended to be
used, plus what we actually guarantee in terms of its semantics.
I'm a bit concerned that the semantics of the attribute currently involve
"try as hard as you can to fold this to a constant, and even ignore
side-effects, but if constant-folding it fails, don't choose this overload"
-- if we change how hard we're willing to try when constant-folding in the
future, it might break code relying on this feature. How essential is it
that we support conditions that have side-effects?
--- include/clang/Sema/TemplateDeduction.h (revision 191171)
+++ include/clang/Sema/TemplateDeduction.h (working copy)
@@ -208,6 +209,10 @@
/// if any.
Expr *getExpr();
+ /// When FailureKind is ovl_fail_enable_if, the attribute which caused
+ /// this function to not be viable.
+ EnableIfAttr *EnableIfAttribute;
Can you reuse the existing Data pointer for this, rather than growing
DeductionFailureInfo?
--- lib/AST/ExprConstant.cpp (revision 191171)
+++ lib/AST/ExprConstant.cpp (working copy)
@@ -5803,26 +5810,35 @@
//
// Otherwise, it returns 0.
//
+ // In the former case, when building with -O, GCC will sometimes return
1 if
+ // a constant numeric value is used as an argument to an inline
function, and
+ // the corresponding parameter is passed to __builtin_constant_p. In the
+ // latter case, it never will. We pretend -O is always specified when
checking
+ // constexpr function parameters.
+ //
// FIXME: GCC also intends to return 1 for literals of aggregate types,
but
// its support for this does not currently work.
if (ArgType->isIntegralOrEnumerationType()) {
- Expr::EvalResult Result;
- if (!Arg->EvaluateAsRValue(Result, Ctx) || Result.HasSideEffects)
+ llvm::SmallVector<PartialDiagnosticAt, 8> Diag;
+ SpeculativeEvaluationRAII Speculate(Info, &Diag);
+
+ Info.EvalStatus.HasSideEffects = false;
+ APValue Result;
+ if (!EvaluateAsRValue(Info, Arg, Result) ||
Info.EvalStatus.HasSideEffects)
return false;
Reusing the current 'Info' here will mean that __builtin_constant_p's
result will depend on the context as well as its operand. That may be what
you intended here, but is a big enough change to be worth separately
calling out (and separately committing). For instance:
constexpr bool f(int n) { return __builtin_constant_p(n); }
constexpr bool a = f(0);
bool b = f(0);
extern int n;
bool c = f(n);
I think, with your patch, we'll guarantee to initialize 'a' to true and 'c'
to false, and 'b' may or may not be 'true'. Prior to your patch, all three
will be false. IIRC, that basically matches g++'s behavior, but it'd be
good to double-check.
You'll also need to turn off the ability to mutate local state during the
evaluation of the subexpression in C++1y mode.
--- lib/Parse/ParseDecl.cpp (revision 191171)
+++ lib/Parse/ParseDecl.cpp (working copy)
@@ -226,6 +228,21 @@
ParseTypeTagForDatatypeAttribute(*AttrName, AttrNameLoc, Attrs,
EndLoc);
return;
}
+ // These may refer to the function arguments, but need to be parsed
early to
+ // participate in determining whether it's a redeclaration.
+ llvm::OwningPtr<ParseScope> PrototypeScope;
+ if (AttrName->isStr("enable_if") && D && D->isFunctionDeclarator()) {
+ DeclaratorChunk::FunctionTypeInfo FTI = D->getFunctionTypeInfo();
+ PrototypeScope.reset(new ParseScope(this,
Scope::FunctionPrototypeScope |
+ Scope::FunctionDeclarationScope |
+ Scope::DeclScope));
+ for (unsigned i = 0; i != FTI.NumArgs; ++i) {
+ ParmVarDecl *Param = cast<ParmVarDecl>(FTI.ArgInfo[i].Param);
+ getCurScope()->AddDecl(Param);
+ if (Param->getDeclName())
+ Actions.IdResolver.AddDecl(Param);
Filling in the scope should be done in Sema/, not in Parse/.
Can we avoid redoing the enable_if check in SemaChecking if we've already
been through overload resolution (and done the checking there)?
--- lib/Sema/SemaOverload.cpp (revision 191171)
+++ lib/Sema/SemaOverload.cpp (working copy)
@@ -1079,6 +1079,21 @@
return true;
}
+ // enable_if attributes are an order-sensitive part of the signature.
+ for (specific_attr_iterator<EnableIfAttr>
+ NewI = New->specific_attr_begin<EnableIfAttr>(),
+ NewE = New->specific_attr_end<EnableIfAttr>(),
+ OldI = Old->specific_attr_begin<EnableIfAttr>(),
+ OldE = Old->specific_attr_end<EnableIfAttr>();
+ NewI != NewE && OldI != OldE; ++NewI, ++OldI) {
+ if (NewI == NewE || OldI == OldE)
+ return true;
+ llvm::FoldingSetNodeID NewID, OldID;
+ NewI->getCond()->Profile(NewID, Context, true);
+ OldI->getCond()->Profile(OldID, Context, true);
+ return !(NewID == OldID);
I don't think you meant to return here in the case where NewID == OldID.
(You're only checking the first enable_if attribute on each function.)
+EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *>
Args,
+ bool MissingImplicitThis) {
+
+ for (specific_attr_iterator<EnableIfAttr>
+ I = Function->specific_attr_begin<EnableIfAttr>(),
+ E = Function->specific_attr_end<EnableIfAttr>(); I != E; ++I) {
+ APValue Result;
+
+ SFINAETrap Trap(*this);
+
+ // Convert the arguments.
+ SmallVector<Expr *, 16> Params;
ConvertedArgs maybe? These are not parameters.
--- test/Sema/enable_if.c (revision 0)
+++ test/Sema/enable_if.c (working copy)
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 %s -verify -Wno-gcc-compat
Presumably this warning flag is for the 'attributes on function
definitions' warning? Maybe we should only provide that for GCC-compatible
attributes. We already suppress it for the thread-safety attributes, IIRC.
[...]
+// This f() is never chosen because the constant evaluator fails on
non-const global, but there's no error for that.
+// FIXME: we should issue an error at the point of the enable_if
declaration
+int global;
+void f(int n) __attribute__((enable_if(global == 0, "chosen when 'global'
is zero"))); // expected-note{{candidate ignored: chosen when 'global' is
zero}}
You can use isPotentialConstantExpr for the interesting half of this (where
the enable_if expression is never constant). I'm not sure what you want to
do with the boring half (where the expression evaluates to 'false' -- or
'true' -- independently of the function's parameters' values or template
arguments).
On Sun, Sep 22, 2013 at 2: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.
>
>
> + 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?
>
>
> 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.
>
> Nick
>
> _______________________________________________
> 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/c90e247a/attachment.html>
More information about the cfe-commits
mailing list