[cfe-dev] Fwd: [patch][libcxx] fix C++98 binders when binding a ref-typed argument

[Jesse Zhang via cfe-dev]

Hi Arthur,

On Wed, Mar 3, 2021 at 5:20 PM Arthur O'Dwyer wrote:
>
> On Wed, Mar 3, 2021 at 8:03 PM Jesse Zhang via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>
> FYI (and in answer to your last question), there is a libc++-specific mailing list at <libcxx-dev at lists.llvm.org>.

Thanks for your prompt response! I *did* try asking on libcxx-dev half a
year ago [1], but there doesn't seem to be a big audience there. I wish
I'd tried cfe-dev sooner.

> The usual workflow is to post patches-for-review to Phabricator at
> https://reviews.llvm.org/differential/
> ; do you have the capability to do that? (Otherwise I dunno, maybe Louis Dionne will take it from here?)

Embarrased to say I haven't tried that (will do). I come from more
old-fashioned communities (Linux / Postgres) where contributions are
patches, and discussions are primarily on IRC / listserv.

>>
>> Here's a minimal repro (obligatory godbolt:
>> https://godbolt.org/z/3zra7s):
>> [...]
>>
>> The attached patch fixes this by converting the
>> arguments before constructing the binder objects.
>
>
> Looks reasonable to me. Your patch seems to be simply improving libc++'s adherence to to the actual C++11 specification of bind1st/bind2nd in
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
>
> - Notice that bind1st/bind2nd did not exist in C++98, so libc++'s support of them in C++98 mode is a pure extension.
> - Notice that bind1st/bind2nd were "deprecated on arrival" in C++11, and are no longer standard C++ as of C++17.
> So you're asking to "fix" an extension, to C++98, which has never existed non-deprecated in the paper standard, which hasn't existed (even in deprecated form) for two whole standard revisions at this point.
> BUT... by the same tokens, the stakes of fixing this must be pretty low, and your patch seems obviously "correct" to me.

I did some due diligence to get my hands on a copy of C++98 (and made
some new friends), and the binders are specified in the standard [2].

>
>> 1. I'd like to include a failing test in the patch, but am not
>> sufficiently familiar with the code base to know where to add one.
>
>
> You should add new *passing* tests, to libcxx/test/std/depr/depr.lib.binders/.
> The bug is that you have some code that SHOULD compile but doesn't (because of the bug), right? So the new test case should test that your code DOES compile.
>

/me nods incessantly.

Sorry for not using clearer words. I meant a test that would fail
*before* the code addition -- it goes without saying it passes after the
fix -- hence the term "failing test".

I was holding onto this email thinking I would send my response with a
new patch, but I'm still struggling a bit with adding a test, mostly
because the standard text actually requires a functional cast, which is
equivalent to a C-style cast, so I'm seeing a fairly simple test now
needing to cover 5*2 = 10 cases (const_cast, static_cast, static_cast +
const_cast, reinterpret_cast, reinterpret_cast + const_cast, double that
for 2nd arg), so I decide to respond with what I have first.

Cheers,
Jesse

[1] https://lists.llvm.org/pipermail/libcxx-dev/2020-August/000920.html
[2] ISO/IEC 14882:1998(E), subsection "20.3.6 Binders [lib.binders]"