patch: new attribute enable_if

[Richard Smith]

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 <nicholas at mxc.ca> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/b75c27ad/attachment.html>