In D142902#4631440, @ldionne wrote:

[Github PR transition cleanup]

I don't know how to say this other than to say that we are reluctant to increase the complexity and the "API surface" (in the general sense of having a contract with users) of the library in order to provide an impression of ABI-compatibility with MSVC. If we wanted to be truly ABI compatible with MSVC in some regards, we would want to have a much more holistic plan than just making sure these function names match and a few data structures happen to have the same layout. That's just too brittle and it creates a haunted graveyard where we'll forever wonder how much leeway we have in making changes to our code because of this presumed ABI compatibility. I also question how much benefit this would provide to users, since I expect the number of people that will try to mix libc++ and the MSVC STL in a way that requires ABI compatibility to be extremely tiny.

Also, generally speaking, mixing stdlibs makes it super easy for users to get into sketchy situations without understanding why. We generally frown upon making changes that suggest it's a good idea. For all these reasons, I believe this patch is not in line with where we want to take the library so I'm going to close it. (Sorry, that requires commandeering and then abandoning which may seem a bit rude, but it's how Phab works)

If there is no parallel backend available, the serial backend should be used instead of not providing anything. I also don't really see a use-case for disabling specific policies.

Thanks for keeping me in the loop.

Sorry for disappearing. That was because of Intel business suspension in Russia. I was not allowed to work on that kind of stuff.

In D142902#4119069, @ldionne wrote:

Have libstdc++ and MSVC concerted to make sure their respective pmr implementations were ABI compatible?

I'm not convinced it's a good idea to guarantee ABI compatibility in any way here, but my main concern right now is that the changes in this patch don't reflect what is claimed in the commit message. monotonic_buffer_resource, etc. are clearly not ABI compatible and polymorphic_resource should probably also be in the versioned namespace. I see no reason to change that.

In D142902#4090931, @philnik wrote:

Our implementation isn't ABI compatible with libstdc++ or the MSVL STL. Putting them in the same namespace just results in subtle bugs.

LGTM. @ldionne, any thoughts on that review?

In D119703#3473061, @ldionne wrote:

Could you rebase onto main and re-upload this? I suspect the failures might go away now that we've changed a few things about the debug mode.

In D103198#3349135, @Mordante wrote:

@rarutyun you first need to commandeer this review, which is in the "Add Action..." drop down.
(AFAIK that requires no special permissions.)

I am trying to update this review to continue enabling of PSTL CI with necessary modifications of the pipeline and fixes in PSTL source/tests, but I cannot do that. Seems like I don't have enough permissions to update the review I am not the author of. I am using web interface and when I am pressing "Update diff" I can only create the new revision and don't have an option to update the current one. Probably it's because I don't have direct Phabricator account (I have been using the GitHub one from the very beginning) or probably the reason is different. Together with @MikeDvorskiy we tried to do the same from his account (using web interface as well) and everything works just fine.

CI is green, @MikeDvorskiy and @rodgert approved that previously. I believe we can finally merge it.

LGTM, waiting for CI to complete

LGTM

Overall, looks good to me, but I believe some small improvements could be done.

In D113998#3148769, @ldionne wrote:

Thanks for the patch! Please provide "Author Name <email@domain>" if you want me to commit this on your behalf.

Add tag_dispatching mechanism for OpenMP backend

In D114624#3155858, @Quuxplusone wrote:

Obviously correct, :shipit:. But some non-blocking nits:
(1) If we have any tests related to PSTL, then surely it's trivial to add a regression test for this. (But I'm not sure we do.)
(2) The indentation would read better as

template <typename _F1, typename _F2>
auto __invoke_if_else(std::false_type, _F1, _F2 __f2)
    -> decltype(__f2())
{

(3) It's mildly weird that __invoke_if and __invoke_if_not return void on both branches, but __invoke_if_else returns auto on both branches. You sure you couldn't just make the return type void?

Thanks for creating the review for this issue. To be honest when implementing this stuff I didn't realize that unique containers may return multiple results in heterogeneous case. Currently I know that because this question is very relevant to the https://wg21.link/P2363 paper.

Thanks @kboyarinov. LGTM.

Do you need help committing the patch? If so, please provide Author Name <email@domain> for me to commit this on your behalf. Also you should get commit access per https://releases.llvm.org/9.0.0/docs/DeveloperPolicy.html#obtaining-commit-access if you plan on submitting more of these fixes. Thanks!

In D111738#3063968, @ldionne wrote:

Thanks a lot for looking into this! I was incredibly surprised to learn that we didn't have tests for std::vector's comparison. It's pretty vexing.

In D99836#3064347, @ldionne wrote:

Leaving the correctness of the backend to @MikeDvorskiy (thanks!), but apart from that this looks good to me.

I've put some small comments but they are definitely not a showstopper for this patch to be merged. Whoever merges it please just make sure to apply those.

Hi everybody,

Should we add test to check availability from <functional> header or it's already covered by this one libcxx/test/std/utilities/function.objects/comparisons/transparent.pass.cpp?

Overall, LGTM except the question with HIDE_FROM_ABI that is already raised and the small comment that I've left.

In D87171#2974508, @avogelsgesang wrote:

is this really P0919R3?
Looks to me more like P0919R0

Using libcxx 12, the example from P0919r3

struct string_hash {
  using transparent_key_equal = std::equal_to<>;  // Pred to use
  using hash_type = std::hash<std::string_view>;  // just a helper local type
  size_t operator()(std::string_view txt) const   { return hash_type{}(txt); }
  size_t operator()(const std::string& txt) const { return hash_type{}(txt); }
  size_t operator()(const char* txt) const        { return hash_type{}(txt); }
};

std::unordered_map<std::string, int, string_hash> map = /* ... */;
map.find("abc");
map.find("def"sv);

doesn't compile. The example from P0919r0

struct string_hash {
  using is_transparent = void;                    // I confirm I know what I am doing
  using hash_type = std::hash<std::string_view>;  // just a helper local type
  size_t operator()(std::string_view txt) const   { return hash_type{}(txt); }
  size_t operator()(const std::string& txt) const { return hash_type{}(txt); }
  size_t operator()(const char* txt) const        { return hash_type{}(txt); }
};

std::unordered_map<std::string, int, string_hash, std::equal_to<>> map = /* ... */;
map.find("abc");
map.find("def"sv);

compiles fine

Rebased one more time.

In D103478#2965197, @ldionne wrote:

@rarutyun Can you please rebase this onto main, it doesn't apply cleanly?

In D103478#2960589, @cjdb wrote:

Also LGTM, but since I'm not a co-approver, we'll need to wait for either @ldionne's approval or a second co-approver.

Hi @cjdb @Quuxplusone. Any progress on this review?

In D103478#2951267, @Quuxplusone wrote:

@rarutyun anything blocking this? Do you need help landing it?