This looks really good. Just a few things:
In LanguageExtensions.rst: "In this way, we pick the more specific" ... missing end of sentence. SemaDeclAttr.cpp: can the Subject and number-of-arguments checking be handled by the common attribute-handling code? 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? On Mon, Dec 30, 2013 at 8:27 PM, Nick Lewycky <[email protected]> wrote: > This is the continuation of this thread: > > http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of- > Mon-20131007/090357.html > > from October 7. > > 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. > > 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: > p1) nothing is known, call regular strnlen > p2) array length is known and maxlen isn't known, call strnlen_chk > p3) array length is known and maxlen is known and it fits > p4) array length is known and maxlen is known and it doesn't fit > > 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. > > 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. > > 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. > > This allows us to implement our four property case with: > i1) a base case with no enable_if attributes > i2) enable_if(__builtin_object_size(s) != -1) > i3) enable_if(__builtin_object_size(s) != -1) enable_if(maxlen <= > __builtin_object_size(s)) > i4) enable_if(__builtin_object_size(s) != -1) enable_if(maxlen > > __builtin_object_size(s)) > > 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! > > 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. > > 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? > > Please review. I'd appreciate making note of which review comments block > submission, and which can be incremental improvements after the initial > commit. > > Nick >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
