<div dir="ltr"><div>This looks really good. Just a few things:</div><div><br></div><div><br></div>In LanguageExtensions.rst:<div><br></div><div>"In this way, we pick the more specific"</div><div><br></div><div>... missing end of sentence.</div>
<div><br></div><div>SemaDeclAttr.cpp: can the Subject and number-of-arguments checking be handled by the common attribute-handling code?</div><div><br></div><div>CheckEnableIf: can you perform the parameter initialization/conversion steps once, rather than once per enable_if attribute? Also, perhaps use std::remove_if rather than the current O(n^2) loop to extract the enable_if attributes (here and later in the same file), or maybe directly iterate over the attribute lists without the additional copy/filter?</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Dec 30, 2013 at 8:27 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is the continuation of this thread:<br>
<br>
<a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131007/090357.html" target="_blank">http://lists.cs.uiuc.edu/<u></u>pipermail/cfe-commits/Week-of-<u></u>Mon-20131007/090357.html</a><br>
<br>
from October 7.<br>
<br>
This update to the patch includes a significant change in semantics. We now support partial ordering of candidates when more than one enable_if attribute is applied to a declaration. The exact semantics of this feature are described in the LanguageExtensions.rst portion of the patch, instead I want to talk about the rationale.<br>

<br>
With the previous form of enable_if, overload resolution would be ambiguous unless all but one expression folded to false. Suppose you have four properties to test:<br>
  p1) nothing is known, call regular strnlen<br>
  p2) array length is known and maxlen isn't known, call strnlen_chk<br>
  p3) array length is known and maxlen is known and it fits<br>
  p4) array length is known and maxlen is known and it doesn't fit<br>
<br>
The four function declarations would need to each test all four of those properties. That's a little ugly, but there's one gotcha that makes it awful: an enable_if expression is a tristate, it could be true, false or could-not-compute (which is observed as false). This means that simply writing __attribute__((enable_if(p1 && !p2 && ...))) doesn't work.<br>

<br>
Instead, we handled this case by making user call __builtin_constant_p() in their enable_if expression to check whether a given argument was a constant, but then that necessitated changes to BCP's constant folding, changes that may make BCP behave differently from GCC.<br>

<br>
The new approach is to add partial function ordering semantics based on the rules for partial ordering of function templates [temp.func.order]. It's the same as "function with largest number of enable_if attributes wins" except that if the n-th enable_if expression on two candidates are not ODR-equivalent, then the two are ambiguous regardless of who has more enable_if attributes.<br>

<br>
This allows us to implement our four property case with:<br>
  i1) a base case with no enable_if attributes<br>
  i2) enable_if(__builtin_object_<u></u>size(s) != -1)<br>
  i3) enable_if(__builtin_object_<u></u>size(s) != -1) enable_if(maxlen <= __builtin_object_size(s))<br>
  i4) enable_if(__builtin_object_<u></u>size(s) != -1) enable_if(maxlen > __builtin_object_size(s))<br>
<br>
In particular, all the tests from the previous version of this patch have been updated to use multiple enable_if attributes where necessary, and none of them use __builtin_constant_p any more. Victory!<br>
<br>
Well, let's not declare victory yet. The implementation has some truly nasty stuff as seen in the patch. The major one is that I can't use specific_attr_iterator in two places I want to because it doesn't guarantee the correct ordering. The replacement code is akin to a lovecraftian horror. Yes, I want to commit it like this first and fix it later.<br>

<br>
I'm also not completely confident in my changes to ExprConstant.cpp. Does adding new EvalModes make sense for this? What is the difference between EM_ConstantExpression and EM_ConstantFold, and could I have made use of that instead of adding new modes?<br>

<br>
Please review. I'd appreciate making note of which review comments block submission, and which can be incremental improvements after the initial commit.<span class="HOEnZb"><font color="#888888"><br>
<br>
Nick<br>
</font></span></blockquote></div><br></div>