patch: new attribute enable_if
Richard Smith
richard at metafoo.co.uk
Mon Oct 7 11:22:24 PDT 2013
On Wed, Oct 2, 2013 at 2:26 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Updated patch attached. Almost all review comments implemented!
Thanks! [Patch has DOS line endings!]
--- docs/LanguageExtensions.rst (revision 191815)
+++ docs/LanguageExtensions.rst (working copy)
+Becuase the enable_if expression is an unevaluated context, there are no
globalstate changes, nor the ability to pass information from the enable_if
*Because
*no global state changes are possible
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 191815)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
+def err_enableif_never_constant_expr : Error<
+ "enable_if attribute expression never produces a constant expression">;
I'd prefer "enable_if" not "enableif" in this diagnostic name.
+def note_ovl_candidate_disabled_by_enable_if_attr : Note<
+ "candidate ignored: %0">;
Maybe "candidate disabled: %0" ?
+bool Expr::EvaluateWithSubstitution(APValue &Value, ASTContext &Ctx,
+ FunctionDecl *Callee,
+ llvm::ArrayRef<const Expr*> Args)
const {
[...]
+ return Evaluate(Value, Info, this) && !Info.EvalStatus.HasSideEffects;
This doesn't match the comment in the .h file, where you say that
side-effects are allowed so long as we can fold to a constant.
+bool Expr::isPotentialConstantExpr(Expr *E,
[...]
+ assert(EvaluateArgs(Args, ArgValues, Info));
Having a side-effecting thing inside an assert is a bit scary. Please
either drop this (because it's a no-op with these arguments) or move it out
of the assert.
- if (OnDefinition && !IsThreadSafetyAttribute(LA.AttrName.getName())) {
+ if (OnDefinition && IsAttributeSupportedByGCC(LA.AttrName.getName())) {
Please commit this part separately.
+/// \brief Wrapper around a case statement checking if AttrName is an
attribute
+/// supported by GCC.
+bool Parser::IsAttributeSupportedByGCC(StringRef AttrName) {
+ return llvm::StringSwitch<bool>(AttrName)
Please generate this with TableGen from the Attr.td file. Ideally we should
also generate the [[gnu::foo]] attribute spellings from the same
information.
+ for (int i = 0, n = Diags.size(); i != n; ++i)
Capital I / N here, please.
--- lib/Sema/SemaOverload.cpp (revision 191815)
+++ lib/Sema/SemaOverload.cpp (working copy)
@@ -1079,6 +1079,22 @@
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;
Your loop condition isn't quite right (&& should be ||): if the lists are
different lengths you'll bail out rather than reaching the 'return true'.
Sema::AddConversionCandidate and AddSurrogateCandidate don't check
enable_if attributes. Those don't seem very interesting (because the only
argument is 'this') but we should probably handle them too.
Richard Smith wrote:
>
>> Hi Nick,
>>
>> This looks really promising. Do you yet know whether this is enough to
>> implement *all* the functionality of -DFORTIFY_SOURCE?
>>
>
> I asked Geremy to try implementing it with the patch I posted, and he says
> he has -DFORTIFY_SOURCE=1 done, and forsees no problems with
> -DFORTIFY_SOURCE=2.
Great!
> 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.
>>
>
> Great point. I've started on this, but I'm not really sure yet what the
> semantics ought to be ...
>
>
> 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?
>>
>
> On the one hand, I will not support anything that actually changes state
> inside the expression. On the other hand, imagine trying to express
> strlen(foo) inside an enable_if expression in C mode where you can't write
> constexpr functions, loops, etc. (No, statement expressions are not allowed
> inside the enable_if attribute, I tried that.)
>
We should support __builtin_strlen on string literals at least, but yeah,
the more general case is tricky. We can't add builtins for everything that
anyone wants. [Maybe we should support __constexpr in C? =)]
> I've asked Geremy to let me know what he's actually using in his
> expressions. If he isn't using anything that isn't a language-defined
> constant expression then that's the obvious way to go.
>
> --- 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?
>>
>
> Done.
>
> --- 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.
>>
>
> All true. And it does match g++.
>
> I am extremely nervous about changing __builtin_constant_p's behaviour,
> even if the result does more closely match gcc in this particular instance.
>
> My ideal would be to have a way to get builtin_constant_p continue to have
> access to the function parameters as I need for enable_if evaluation with
> no other visible changes. It looks like doing that would require making a
> copy of the EvalInfo and that would break all the CallStackFrames which
> hold it by reference, so I'd need to copy them too, yuck.
>
> What do you think I should do here? Try to land this one bit on its own?
> Try to make a deep copy of the EvalInfo state? Something more clever?
My preference would be to land this part on its own, since this seems to
improve g++ compatibility. Alternatively, we could add a special case for
__builtin_constant_p(x) where 'x' is a function parameter in an enable_if
attribute, but that seems a little inelegant.
You'll also need to turn off the ability to mutate local state during
>> the evaluation of the subexpression in C++1y mode.
>>
>
> Can I punt this to a future patch? It might turn out to be a small number
> of lines of code, but I'm worried about the cognitive load of adding yet
> another "thread foo through ExprConstant" getting into this patch.
>
I'm not especially happy for __builtin_constant_p to be temporarily broken
in this way.
> --- lib/Parse/ParseDecl.cpp(**revision 191171)
>> +++ lib/Parse/ParseDecl.cpp(**working copy)
>>
>> @@ -226,6 +228,21 @@
>> ParseTypeTagForDatatypeAttribu**te(*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/.
>>
>
> Done.
>
>
> Can we avoid redoing the enable_if check in SemaChecking if we've
>> already been through overload resolution (and done the checking there)?
>>
>
> Done!
>
> --- 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.)
>>
>
> Whoops! I had that right previously, then broke it during self-review.
>
>
> +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.
>>
>
> Done.
>
> --- 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.
>>
>
> Done. We now have Parser::**IsAttributeSupportedByGCC.
>
>
> [...]
>> +// 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).
>>
>
> Thanks for the pointer, I've implemented this.
>
> I'm okay with expressions that evaluate to true or to false. It could be
> reasonable if it's expanded from a preprocessor macro or somesuch.
>
> It'd be nice to have warnings for "{none|two or more} of the overloads are
> enabled" but I think because of the way we parse we'd get the errors at the
> call-sites before we could detect this.
>
> Nick
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131007/ea63e9ef/attachment.html>
More information about the cfe-commits
mailing list