<div dir="ltr"><div>On Wed, Oct 2, 2013 at 2:26 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br></div><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Updated patch attached. Almost all review comments implemented!</blockquote>
<div><br></div><div>Thanks! [Patch has DOS line endings!]</div><div><br></div><div><div>--- docs/LanguageExtensions.rst<span class="" style="white-space:pre"> </span>(revision 191815)</div><div>+++ docs/LanguageExtensions.rst<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div><div>+Becuase the enable_if expression is an unevaluated context, there are no globalstate changes, nor the ability to pass information from the enable_if</div><div><br></div><div>*Because</div><div>*no global state changes are possible</div>
<div><br></div></div><div><br></div><div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span class="" style="white-space:pre"> </span>(revision 191815)</div><div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div><div>+def err_enableif_never_constant_expr : Error<</div><div>+ "enable_if attribute expression never produces a constant expression">;</div></div><div><br></div><div>I'd prefer "enable_if" not "enableif" in this diagnostic name.</div>
<div><br></div><div><div>+def note_ovl_candidate_disabled_by_enable_if_attr : Note<</div><div>+ "candidate ignored: %0">;</div></div><div><br></div><div>Maybe "candidate disabled: %0" ?</div><div>
<br></div><div><br></div><div><div>+bool Expr::EvaluateWithSubstitution(APValue &Value, ASTContext &Ctx,</div><div>+ FunctionDecl *Callee,</div><div>+ llvm::ArrayRef<const Expr*> Args) const {</div>
<div>[...]</div><div>+ return Evaluate(Value, Info, this) && !Info.EvalStatus.HasSideEffects;<br></div></div><div><br></div><div>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.</div>
<div><br></div><div><div>+bool Expr::isPotentialConstantExpr(Expr *E,</div></div><div>[...]</div><div><div>+ assert(EvaluateArgs(Args, ArgValues, Info));</div></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div><div>- if (OnDefinition && !IsThreadSafetyAttribute(LA.AttrName.getName())) {</div><div>+ if (OnDefinition && IsAttributeSupportedByGCC(LA.AttrName.getName())) {</div></div>
<div><br></div><div>Please commit this part separately.</div><div><br></div><div><div>+/// \brief Wrapper around a case statement checking if AttrName is an attribute</div><div>+/// supported by GCC.</div><div>+bool Parser::IsAttributeSupportedByGCC(StringRef AttrName) {</div>
<div>+ return llvm::StringSwitch<bool>(AttrName)</div></div><div><br></div><div>Please generate this with TableGen from the Attr.td file. Ideally we should also generate the [[gnu::foo]] attribute spellings from the same information.</div>
<div><br></div><div><br></div><div><div>+ for (int i = 0, n = Diags.size(); i != n; ++i)</div></div><div><br></div><div>Capital I / N here, please.</div><div><div><br></div><div><br></div><div>--- lib/Sema/SemaOverload.cpp<span class="" style="white-space:pre"> </span>(revision 191815)</div>
<div>+++ lib/Sema/SemaOverload.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div><div>@@ -1079,6 +1079,22 @@</div><div> return true;</div><div> }</div><div> </div><div>+ // enable_if attributes are an order-sensitive part of the signature.</div>
<div>+ for (specific_attr_iterator<EnableIfAttr></div><div>+ NewI = New->specific_attr_begin<EnableIfAttr>(),</div><div>+ NewE = New->specific_attr_end<EnableIfAttr>(),</div><div>+ OldI = Old->specific_attr_begin<EnableIfAttr>(),</div>
<div>+ OldE = Old->specific_attr_end<EnableIfAttr>();</div><div>+ NewI != NewE && OldI != OldE; ++NewI, ++OldI) {</div><div>+ if (NewI == NewE || OldI == OldE)</div><div>+ return true;</div>
</div></div><div><br></div><div>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'.</div><div><br></div>
<div><br></div><div>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.</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
Richard Smith wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Nick,<br>
<br>
This looks really promising. Do you yet know whether this is enough to<br>
implement *all* the functionality of -DFORTIFY_SOURCE?<br>
</blockquote>
<br></div>
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.</blockquote><div><br></div><div>Great!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
One big thing missing here is documentation. That should serve to both<br>
motivate the feature (and explain why it's sufficiently valuable for us<br>
to have and maintain this extension), and to describe how it's intended<br>
to be used, plus what we actually guarantee in terms of its semantics.<br>
</blockquote>
<br></div>
Great point. I've started on this, but I'm not really sure yet what the semantics ought to be ...<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I'm a bit concerned that the semantics of the attribute currently<br>
involve "try as hard as you can to fold this to a constant, and even<br>
ignore side-effects, but if constant-folding it fails, don't choose this<br>
overload" -- if we change how hard we're willing to try when<br>
constant-folding in the future, it might break code relying on this<br>
feature. How essential is it that we support conditions that have<br>
side-effects?<br>
</blockquote>
<br></div>
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.)<br>
</blockquote><div><br></div><div>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? =)]<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--- include/clang/Sema/<u></u>TemplateDeduction.h(revision 191171)<br>
+++ include/clang/Sema/<u></u>TemplateDeduction.h(working copy)<div class="im"><br>
@@ -208,6 +209,10 @@<br>
/// if any.<br>
Expr *getExpr();<br>
+ /// When FailureKind is ovl_fail_enable_if, the attribute which caused<br>
+ /// this function to not be viable.<br>
+ EnableIfAttr *EnableIfAttribute;<br>
<br>
Can you reuse the existing Data pointer for this, rather than growing<br>
DeductionFailureInfo?<br>
</div></blockquote>
<br>
Done.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--- lib/AST/ExprConstant.cpp(<u></u>revision 191171)<br>
+++ lib/AST/ExprConstant.cpp(<u></u>working copy)<div><div class="h5"><br>
@@ -5803,26 +5810,35 @@<br>
//<br>
// Otherwise, it returns 0.<br>
//<br>
+ // In the former case, when building with -O, GCC will sometimes<br>
return 1 if<br>
+ // a constant numeric value is used as an argument to an inline<br>
function, and<br>
+ // the corresponding parameter is passed to __builtin_constant_p. In the<br>
+ // latter case, it never will. We pretend -O is always specified when<br>
checking<br>
+ // constexpr function parameters.<br>
+ //<br>
// FIXME: GCC also intends to return 1 for literals of aggregate<br>
types, but<br>
// its support for this does not currently work.<br>
if (ArgType-><u></u>isIntegralOrEnumerationType()) {<br>
- Expr::EvalResult Result;<br>
- if (!Arg->EvaluateAsRValue(<u></u>Result, Ctx) || Result.HasSideEffects)<br>
+ llvm::SmallVector<<u></u>PartialDiagnosticAt, 8> Diag;<br>
+ SpeculativeEvaluationRAII Speculate(Info, &Diag);<br>
+<br>
+ Info.EvalStatus.HasSideEffects = false;<br>
+ APValue Result;<br>
+ if (!EvaluateAsRValue(Info, Arg, Result) ||<br>
Info.EvalStatus.<u></u>HasSideEffects)<br>
return false;<br>
<br>
<br>
Reusing the current 'Info' here will mean that __builtin_constant_p's<br>
result will depend on the context as well as its operand. That may be<br>
what you intended here, but is a big enough change to be worth<br>
separately calling out (and separately committing). For instance:<br>
<br>
constexpr bool f(int n) { return __builtin_constant_p(n); }<br>
constexpr bool a = f(0);<br>
bool b = f(0);<br>
extern int n;<br>
bool c = f(n);<br>
<br>
I think, with your patch, we'll guarantee to initialize 'a' to true and<br>
'c' to false, and 'b' may or may not be 'true'. Prior to your patch, all<br>
three will be false. IIRC, that basically matches g++'s behavior, but<br>
it'd be good to double-check.<br>
</div></div></blockquote>
<br>
All true. And it does match g++.<br>
<br>
I am extremely nervous about changing __builtin_constant_p's behaviour, even if the result does more closely match gcc in this particular instance.<br>
<br>
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.<br>
<br>
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?</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
You'll also need to turn off the ability to mutate local state during<br>
the evaluation of the subexpression in C++1y mode.<br>
</blockquote>
<br></div>
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.<br>
</blockquote><div><br></div><div>I'm not especially happy for __builtin_constant_p to be temporarily broken in this way.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--- lib/Parse/ParseDecl.cpp(<u></u>revision 191171)<br>
+++ lib/Parse/ParseDecl.cpp(<u></u>working copy)<div class="im"><br>
@@ -226,6 +228,21 @@<br>
ParseTypeTagForDatatypeAttribu<u></u>te(*AttrName, AttrNameLoc, Attrs,<br>
EndLoc);<br>
return;<br>
}<br>
+ // These may refer to the function arguments, but need to be parsed<br>
early to<br>
+ // participate in determining whether it's a redeclaration.<br>
+ llvm::OwningPtr<ParseScope> PrototypeScope;<br>
+ if (AttrName->isStr("enable_if") && D && D->isFunctionDeclarator()) {<br>
+ DeclaratorChunk::<u></u>FunctionTypeInfo FTI = D->getFunctionTypeInfo();<br>
+ PrototypeScope.reset(new ParseScope(this,<br>
Scope::FunctionPrototypeScope |<br>
+ Scope::<u></u>FunctionDeclarationScope |<br>
+ Scope::DeclScope));<br>
+ for (unsigned i = 0; i != FTI.NumArgs; ++i) {<br>
+ ParmVarDecl *Param = cast<ParmVarDecl>(FTI.ArgInfo[<u></u>i].Param);<br>
+ getCurScope()->AddDecl(Param);<br>
+ if (Param->getDeclName())<br>
+ Actions.IdResolver.AddDecl(<u></u>Param);<br>
<br>
Filling in the scope should be done in Sema/, not in Parse/.<br>
</div></blockquote>
<br>
Done.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Can we avoid redoing the enable_if check in SemaChecking if we've<br>
already been through overload resolution (and done the checking there)?<br>
</blockquote>
<br></div>
Done!<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--- lib/Sema/SemaOverload.cpp(<u></u>revision 191171)<br>
+++ lib/Sema/SemaOverload.cpp(<u></u>working copy)<div class="im"><br>
@@ -1079,6 +1079,21 @@<br>
return true;<br>
}<br>
+ // enable_if attributes are an order-sensitive part of the signature.<br>
+ for (specific_attr_iterator<<u></u>EnableIfAttr><br>
+ NewI = New->specific_attr_begin<<u></u>EnableIfAttr>(),<br>
+ NewE = New->specific_attr_end<<u></u>EnableIfAttr>(),<br>
+ OldI = Old->specific_attr_begin<<u></u>EnableIfAttr>(),<br>
+ OldE = Old->specific_attr_end<<u></u>EnableIfAttr>();<br>
+ NewI != NewE && OldI != OldE; ++NewI, ++OldI) {<br>
+ if (NewI == NewE || OldI == OldE)<br>
+ return true;<br>
+ llvm::FoldingSetNodeID NewID, OldID;<br>
+ NewI->getCond()->Profile(<u></u>NewID, Context, true);<br>
+ OldI->getCond()->Profile(<u></u>OldID, Context, true);<br>
+ return !(NewID == OldID);<br>
<br>
I don't think you meant to return here in the case where NewID == OldID.<br>
(You're only checking the first enable_if attribute on each function.)<br>
</div></blockquote>
<br>
Whoops! I had that right previously, then broke it during self-review.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+EnableIfAttr *Sema::CheckEnableIf(<u></u>FunctionDecl *Function, ArrayRef<Expr<br>
*> Args,<br>
+ bool MissingImplicitThis) {<br>
+<br>
+ for (specific_attr_iterator<<u></u>EnableIfAttr><br>
+ I = Function->specific_attr_begin<<u></u>EnableIfAttr>(),<br>
+ E = Function->specific_attr_end<<u></u>EnableIfAttr>(); I != E; ++I) {<br>
+ APValue Result;<br>
+<br>
+ SFINAETrap Trap(*this);<br>
+<br>
+ // Convert the arguments.<br>
+ SmallVector<Expr *, 16> Params;<br>
<br>
ConvertedArgs maybe? These are not parameters.<br>
</blockquote>
<br></div>
Done.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--- test/Sema/enable_if.c(revision 0)<br>
+++ test/Sema/enable_if.c(working copy)<div class="im"><br>
@@ -0,0 +1,93 @@<br>
+// RUN: %clang_cc1 %s -verify -Wno-gcc-compat<br>
<br>
<br>
Presumably this warning flag is for the 'attributes on function<br>
definitions' warning? Maybe we should only provide that for<br>
GCC-compatible attributes. We already suppress it for the thread-safety<br>
attributes, IIRC.<br>
</div></blockquote>
<br>
Done. We now have Parser::<u></u>IsAttributeSupportedByGCC.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
[...]<br>
+// This f() is never chosen because the constant evaluator fails on<br>
non-const global, but there's no error for that.<br>
+// FIXME: we should issue an error at the point of the enable_if<br>
declaration<br>
+int global;<br>
+void f(int n) __attribute__((enable_if(<u></u>global == 0, "chosen when<br>
'global' is zero"))); // expected-note{{candidate ignored: chosen when<br>
'global' is zero}}<br>
<br>
<br>
You can use isPotentialConstantExpr for the interesting half of this<br>
(where the enable_if expression is never constant). I'm not sure what<br>
you want to do with the boring half (where the expression evaluates to<br>
'false' -- or 'true' -- independently of the function's parameters'<br>
values or template arguments).<br>
</blockquote>
<br></div>
Thanks for the pointer, I've implemented this.<br>
<br>
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.<br>
<br>
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.<span class=""><font color="#888888"><br>
<br>
Nick<br>
</font></span></blockquote></div><br></div></div>