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

[Jonathan Wakely]

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.

>Thanks for the prototyping!
>
>
>-- 
>Regards,
>Tim Shen