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

[Arthur O'Dwyer via cfe-dev]

On Wed, Mar 3, 2021 at 8:03 PM Jesse Zhang via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> Hi LLVM hackers,
>
> PFA a tiny patch that fixes a bug I found in libcxx.
>

FYI (and in answer to your last question), there is a libc++-specific
mailing list at <libcxx-dev at lists.llvm.org>.
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?)


> 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.

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.

2. I tried running clang-format over my patch but noticed it results in
> a massive diff so I held off formatting, is that acceptable?
>

Yes.


> 3. I tried doing some archaeology to see whether this was a regression,
> but I could only find a "genesis commit" where Howard imported libcxx
> wholesale on May 11 2010 ("libcxx initial import", SVN r103490, Git
> monorepo commit 3e519524c1186). Is the version control history before
> that published somewhere?
>

I don't know, and am curious; but in this particular case it certainly
doesn't matter to the fate of your patch.

HTH,
Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210303/e804ebd6/attachment.html>