<div dir="ltr"><div dir="ltr">On Wed, Mar 3, 2021 at 8:03 PM Jesse Zhang via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi LLVM hackers,<br>
<br>
PFA a tiny patch that fixes a bug I found in libcxx.<br></blockquote><div><br>FYI (and in answer to your last question), there is a libc++-specific mailing list at <<a href="mailto:libcxx-dev@lists.llvm.org">libcxx-dev@lists.llvm.org</a>>.<br></div><div>The usual workflow is to post patches-for-review to Phabricator at</div><div><a href="https://reviews.llvm.org/differential/">https://reviews.llvm.org/differential/</a><br></div><div>; do you have the capability to do that? (Otherwise I dunno, maybe Louis Dionne will take it from here?)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Here's a minimal repro (obligatory godbolt:<br>
<a href="https://godbolt.org/z/3zra7s" rel="noreferrer" target="_blank">https://godbolt.org/z/3zra7s</a>):<br>[...]<br>
<br>The attached patch fixes this by converting the<br>
arguments before constructing the binder objects.<br></blockquote><div><br></div><div>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</div><div><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf" target="_blank">http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf</a><br></div><div><br></div><div>- Notice that bind1st/bind2nd did not exist in C++98, so libc++'s support of them in C++98 mode is a pure extension.</div><div>- Notice that bind1st/bind2nd were "deprecated on arrival" in C++11, and are no longer standard C++ as of C++17.</div><div>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.<br></div><div>BUT... by the same tokens, the stakes of fixing this must be pretty low, and your patch seems obviously "correct" to me.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
1. I'd like to include a failing test in the patch, but am not<br>
sufficiently familiar with the code base to know where to add one.</blockquote><div><br></div><div>You should add new *passing* tests, to libcxx/test/std/depr/depr.lib.binders/.</div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2. I tried running clang-format over my patch but noticed it results in<br>
a massive diff so I held off formatting, is that acceptable?<br></blockquote><div><br></div><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">3. I tried doing some archaeology to see whether this was a regression,<br>
but I could only find a "genesis commit" where Howard imported libcxx<br>
wholesale on May 11 2010 ("libcxx initial import", SVN r103490, Git<br>
monorepo commit 3e519524c1186). Is the version control history before<br>
that published somewhere?<br></blockquote><div><br></div><div>I don't know, and am curious; but in this particular case it certainly doesn't matter to the fate of your patch.</div><div><br></div><div>HTH,</div><div>Arthur</div></div></div>