Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<memory>: Workaround for MSVC bug on operator<=> (shared_ptr only) #3647

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

Fixes #3646.

I think similar handling for unique_ptr is definitely wanted, but it seems that we can't achieve a complete workaround for unique_ptr due to the arbitrability of unique_ptr::pointer.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner April 10, 2023 17:03
I must have been too tired to correctly write anything...
@AlexGuteniev
Copy link
Contributor

we can't achieve a complete workaround for unique_ptr due to the arbitrability of unique_ptr::pointer.

If the workaround is very much wanted, think we can specialize the workaround. But I'd gave up and waited for the compiler fix instead.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 10, 2023
@StephanTLavavej
Copy link
Member

I audited the library to find all other affected locations:

C:\Temp>type meow.cpp
#include <cassert>
#include <compare>
#include <iterator>
#include <memory>
#include <optional>
using namespace std;

struct NoDeleter {
    template <typename T>
    void operator()(const T&) {}
};

int main() {
    int arr[2]{};
    const int* p = arr;
    void* q      = arr + 1;

    static_assert(three_way_comparable_with<const int*, void*>);
    assert(compare_three_way{}(p, q) == strong_ordering::less);
    assert(basic_const_iterator{p} <=> q == strong_ordering::less);
    assert((unique_ptr<const int, NoDeleter>{p} <=> unique_ptr<void, NoDeleter>{q} == strong_ordering::less));
    assert(optional{p} <=> optional{q} == strong_ordering::less);
    assert(optional{p} <=> q == strong_ordering::less);
}
C:\Temp>clang-cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
meow.cpp(18): error C2607: static assertion failed
[...]
  • The three_way_comparable_with concept would be annoying (and potentially fragile) to apply the workaround to. Presumably, most uses of three_way_comparable_with would be immediately followed by attempting to directly use the spaceship operator, so applying the workaround would serve little purpose.
  • The compare_three_way function object wraps the spaceship operator in function object form. Aside from being constrained by three_way_comparable_with, while we could apply the workaround, concealing compiler bugs isn't the job of this function object.
  • basic_const_iterator is both very new and this usage is very squirrelly 🐿️. void* isn't an iterator (can't be incremented), so the comparison has to be written against a plain void*. This is extremely pathological and nobody will encounter it.
  • unique_ptr differs from shared_ptr in that it's pretty hard to get a unique_ptr to void - you have to do something about the deleter. For this reason, I believe that this scenario is much more difficult to encounter. (shared_ptr<void> is perfectly cromulent and handles destruction automatically.)
  • optional is a popular type, and both of its spaceships are affected. However, an optional pointer is fairly unusual (as pointers already have a null state). Not unthinkable, but unlikely.

Therefore, I believe that limiting the workaround to shared_ptr seems reasonable. If we receive further reports, I agree with @AlexGuteniev that we should simply ask the compiler team to fix the bug (the library can't endlessly work around compiler bugs, we do so much already).

@StephanTLavavej StephanTLavavej self-assigned this Apr 12, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 5f1a105 into microsoft:main Apr 14, 2023
@StephanTLavavej
Copy link
Member

Thanks for reporting this compiler bug and adding a workaround to the most affected location! 🐞 🛸 🛠️

@frederick-vs-ja frederick-vs-ja deleted the shared_ptr-3way-workaround branch April 14, 2023 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<memory>: shared_ptr comparison (<=>)
4 participants