[libstdc++/71500] make back reference work with icase

[Jonathan Wakely]

On 19/09/17 15:38 +0100, Jonathan Wakely wrote:
>On 18/09/17 16:54 -0700, Tim Shen wrote:
>>On Mon, Sep 18, 2017 at 4:01 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>On 18/09/17 10:58 -0700, Tim Shen via libstdc++ wrote:
>>>>
>>>>On Mon, Sep 18, 2017 at 10:26 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>wrote:
>>>>>>
>>>>>>We need to rewrite this to check the lengths are equal first, and then
>>>>>>call the 3-argument version of std::equal.
>>>>>>
>>>>>>Alternatively, we could move the implementation of the C++14
>>>>>>std::equal overloads to __equal and make that available for C++11.
>>>>>>I'll try that.
>>>>>
>>>>>
>>>>>
>>>>>Here's a proof of concept patch for that. It's a bit ugly.
>>>>
>>>>
>>>>Instead of having iterator tags in the interface, we can probe the
>>>>random-access-ness inside __equal4/__equal4_p, can't we? It's similar
>>>>to the existing "if (_RAIters()) { ... }".
>>>>
>>>>I'd expect the patches to be renaming the current implementations and
>>>>adding wrappers, instead of adding new implementations.
>>>
>>>
>>>Well I decided to split the existing functions up and use tag
>>>dispatching, which is conceptually cleaner anyway. But as the
>>>RandomAccessIterator version doesn't need any operations that aren't
>>>valid for other categories, it's not strictly necessary. The tag
>>>dispatching version should generate slightly smaller code for
>>>unoptimized builds, but that's not very important.
>>
>>Unoptimized builds don't inline small functions, therefore the first
>>patch generate two weak symbols, instead of one by the second patch.
>
>Two small functions that only do the necessary work, rather than one
>large function that has a branch for RAIters even when it can never be
>taken.
>
>>It's unclear to me how would number of symbols penalize the
>>performance/binary size.
>
>People who care about performance or binary size should be optimizing,
>and in that case the RAIters branch will be known at compile-time and
>the dead code should get removed, and the wrapper functions inlined.
>
>>>Here's the patch doing it as you suggest. We can't call the new
>>>functions __equal because t hat name is already taken by a helper
>>>struct, hence __equal4.
>>>
>>>Do you prefer this version?
>>
>>Yes, I prefer this version for readability reasons:
>>1) subjectively, less scattered code; and
>>2) ideally I want `if constexpr (...)`), the if version is closer.
>
>Yes, we could add _GLIBCXX17_CONSTEXPR there, but I'm not sure it's
>worth doing.
>
>3) The calls to __equal4 in _Backref_matcher are simpler.
>
>>I agree that it's not a big difference. I just wanted to point out the
>>small difference. I'm fine with either version.
>
>I'll commit the second version.

Here's what I've committed, with a minimal test to catch this
happening in future.

I'll re-run the full set of test variations.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 7756 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20170919/aabd0847/attachment.bin>