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>: !_Get_ptr() in the destructors of inout_ptr_t and out_ptr_t _should_ be correct #4964

Open
frederick-vs-ja opened this issue Sep 18, 2024 · 5 comments
Labels
blocked on LWG Waiting for WG21 to tell us what to do

Comments

@frederick-vs-ja
Copy link
Contributor

Describe the bug

Currently !_Get_ptr() is used in the destructors of inout_ptr_t and out_ptr_t.

STL/stl/inc/memory

Lines 4409 to 4411 in a26f4ad

if (!_Get_ptr()) {
return;
}

STL/stl/inc/memory

Lines 4328 to 4330 in a26f4ad

if (!_Get_ptr()) {
return;
}

However, it seems that nothing in Cpp17NullablePointer ([nullablepointer.requirements]) requires !p to be semantically equivalent to p != nullptr or even to be well-formed. E.g. a type with a deleted member operator! (the NoExclamation type shown below) can still meet the Cpp17NullablePointer requirements.

#include <memory>

struct NoExclamation {
    using pointer = NoExclamation;

    void* p_{};

    NoExclamation() = default;
    constexpr NoExclamation(decltype(nullptr)) noexcept {}

    friend bool operator==(NoExclamation, NoExclamation) = default;
    friend constexpr bool operator==(NoExclamation lhs, decltype(nullptr)) noexcept
    {
        return lhs.p_ == nullptr;
    }

    constexpr explicit operator bool() const noexcept
    {
        return p_ != nullptr;
    }

    void operator!() const = delete; // HERE!
};

int main()
{
    NoExclamation ne{};
    std::out_ptr(ne);
}

Currently no implementation accepts the example due to the deleted operator!.

Command-line test case

Godbolt link

Expected behavior

Perhaps the program should compile.

STL version

a26f4ad (probably every version after #1998)

Additional context

As libc++ and libstdc++ also expect usable operator!, should there be an LWG issue to make current implementation strategies comforming?

@cpplearner
Copy link
Contributor

[nullablepointer.requirements]:

An object p of type P can be contextually converted to bool. The effect shall be as if p != nullptr had been evaluated in place of p.

Pedantically, this doesn't require !p to be meaningful (nor p && p or p || p, nor even implicit conversion to bool). But the intent should be quite clear to a non-pedant.

@frederick-vs-ja
Copy link
Contributor Author

[nullablepointer.requirements]:

An object p of type P can be contextually converted to bool. The effect shall be as if p != nullptr had been evaluated in place of p.

Pedantically, this doesn't require !p to be meaningful (nor p && p or p || p, nor even implicit conversion to bool). But the intent should be quite clear to a non-pedant.

Sounds like that contextual version of boolean-testability is wanted.

@CaseyCarter CaseyCarter added the bug Something isn't working label Sep 18, 2024
@CaseyCarter CaseyCarter changed the title <memory>: Should we avoid !_Get_ptr() in the destructors of inout_ptr_t and out_ptr_t? <memory>: We should avoid !_Get_ptr() in the destructors of inout_ptr_t and out_ptr_t Sep 18, 2024
@StephanTLavavej StephanTLavavej added LWG issue needed A wording defect that should be submitted to LWG as a new issue and removed bug Something isn't working labels Sep 18, 2024
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting. The fact that the Majestic Three implementations all want !p to behave reasonably is a strong indication to us that Cpp17NullablePointer should be requiring boolean-testable ([concept.booleantestable]) and that we should file an LWG issue rather than attempting to contort our implementation to handle absurdly pathological types.

@CaseyCarter CaseyCarter changed the title <memory>: We should avoid !_Get_ptr() in the destructors of inout_ptr_t and out_ptr_t <memory>: !_Get_ptr() in the destructors of inout_ptr_t and out_ptr_t _should_ be correct Sep 18, 2024
@frederick-vs-ja
Copy link
Contributor Author

Cpp17NullablePointer should be requiring boolean-testable ([concept.booleantestable])

I guess it's intentional that implicit convertibility to bool (which is required in boolean-testable) is not required - all of unique_ptr, shared_ptr, and exception_ptr (in Majestic Three implementations) have explicit conversion functions to bool.

@StephanTLavavej
Copy link
Member

This is LWG-4155.

(Good point about not wanting implicit conversions to bool, forgot about that.)

@StephanTLavavej StephanTLavavej added blocked on LWG Waiting for WG21 to tell us what to do and removed LWG issue needed A wording defect that should be submitted to LWG as a new issue labels Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked on LWG Waiting for WG21 to tell us what to do
Projects
None yet
Development

No branches or pull requests

4 participants