This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Qualify make_pair in searcher implementations to prevent ADL
ClosedPublic

Authored by logan-5 on Jan 13 2020, 12:37 PM.

Details

Summary

This patch adds _VSTD:: to some calls to make_pair inside the implementations of searchers, to prevent things exploding if there is a make_pair in an associated namespace of a user-defined type. https://godbolt.org/z/xAFG98

I didn't fix (or add tests to) the Boyer-Moore or Boyer-Moore-Horspool searchers in experimental/, since I couldn't get those to work with user-defined types at all. After defining the hash function properly, there seems to be an issue with a vector<value_type> __scratch in a helper function that I don't understand. See https://godbolt.org/z/heJ75B -- the same code works with libstdc++. If this is a separate issue, I'd be happy to write up a bug. (Also, I don't even know... is the experimental/ stuff still actively supported/bugfixed/etc?)

I did, however, add tests to the regular (currently XFAILed) BM and BMH tests in utilities.

I found this (and a couple other issues, patches forthcoming) by running https://reviews.llvm.org/D72282 over libc++ locally.

Diff Detail

Event Timeline

logan-5 created this revision.Jan 13 2020, 12:37 PM

there seems to be an issue with a vector<value_type> __scratch in a helper function that I don't understand

My relatively uneducated guess is that it should have been a vector<size_t> __scratch all along. If you make that change, then your test case compiles. Also, if you're searching in a string of char, then char is smaller than size_t and bad stuff happens:
https://godbolt.org/z/i52zkN triggers an assertion failure corrupted size vs. prev_size and I can't even figure out where that message is coming from!

logan-5 updated this revision to Diff 238615.Jan 16 2020, 2:07 PM

Fixed some wrong author information in the patch.

EricWF accepted this revision.Jan 17 2020, 12:14 PM

I don't know that you needed to duplicate all the tests, but this works for me.

LGTM.

libcxx/test/std/utilities/function.objects/func.search/func.search.bmh/pred.pass.cpp
70

delete;

This revision is now accepted and ready to land.Jan 17 2020, 12:14 PM
logan-5 marked an inline comment as done.Jan 22 2020, 10:33 AM

Thanks. In that case I'll need help committing this.

libcxx/test/std/utilities/function.objects/func.search/func.search.bmh/pred.pass.cpp
70

What's the advantage of = delete;ing this function versus what I've done here?

@EricWF ping! (@logan-5 says he needs someone to land this patch, and I think that's you.)

@logan-5, you mentioned that you have found other cases of unintended ADL by applying the clang-tidy check in D72282, right? But those patches have not yet been uploaded to Phab? Is there any help I can offer that would advance that work?

@logan-5, you mentioned that you have found other cases of unintended ADL by applying the clang-tidy check in D72282, right? But those patches have not yet been uploaded to Phab? Is there any help I can offer that would advance that work?

Hi @Quuxplusone, thanks a lot for the ping. I did indeed promise more patches for more cases found by that clang-tidy check, but I haven't gotten a chance to implement them yet. I think I was waiting for this one to land completely for some reason.

I think I'll be able to get to it this weekend... unless somebody beats me to it. :)

Pinging this again.

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 5 2020, 4:26 PM
This revision now requires review to proceed.May 5 2020, 4:26 PM
EricWF requested changes to this revision.May 5 2020, 6:00 PM

There are some simplifications suggested in the inline comments.
Once those are addressed, this LGTM.

libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search.pass.cpp
51

Please add a single additional test for each function that was fixed.
Separation of test concerns is better than carpet bombing the tests to prevent ADL,
and it makes the added test more understandable.

Once that's done this diff should be much smaller than it currently is.

63

Make this a non-member.

69

= delete

This revision now requires changes to proceed.May 5 2020, 6:00 PM
logan-5 updated this revision to Diff 262308.May 6 2020, 1:06 AM
logan-5 marked 5 inline comments as done.

Pruned down tests to the essentials.

libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search.pass.cpp
63

No problem, but curious about the reason.

69

Also curious. This doesn't seem like it makes much of a difference, although I suppose = delete communicates better that we don't intend to call this in the tests anywhere.

logan-5 updated this revision to Diff 262412.May 6 2020, 10:19 AM

Formatting.

logan-5 marked an inline comment as done.May 6 2020, 10:21 AM
logan-5 added inline comments.
libcxx/test/std/algorithms/alg.nonmodifying/alg.search/search.pass.cpp
113

I used this formatting (and the rest of the formatting in this diff) to be consistent with the pre-existing surrounding code in this file. The clang-tidy lint check is complaining here though. Is it more important to make those checks pass, or be consistent with the surrounding file? Genuine question.

EricWF accepted this revision.May 6 2020, 1:28 PM

LGTM.

Thanks!

This revision is now accepted and ready to land.May 6 2020, 1:28 PM

Thanks. I'll need committing help, if this is good to go.

Thanks for the patch! Committed as

commit 5b4a98eb58aa7672bbdc5e31e573f8f1220fc9c7
Author: Logan Smith <logan.r.smith0@gmail.com>
Date:   Thu May 7 11:54:25 2020 -0400

    [libcxx] Qualify make_pair in searcher implementations to prevent ADL

    This patch adds `_VSTD::` to some calls to `make_pair` inside the
    implementations of searchers, to prevent things exploding if there is
    a make_pair in an associated namespace of a user-defined type.
    https://godbolt.org/z/xAFG98

    Differential Revision: https://reviews.llvm.org/D72640
This revision was automatically updated to reflect the committed changes.