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

unique_impl_ptr: fix bug in operator< #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

meator
Copy link
Contributor

@meator meator commented Jan 23, 2025

EDIT: Also fixed both operator<s for std::nullptr_t.

A non-const std::less instantiation cannot be used for const data.

Sample program showcasing the problem:

#include "spimpl.h"

int main()
{
    auto first = spimpl::make_unique_impl<int>();
    auto second = spimpl::make_unique_impl<int>();
    first < second;
}
gcc error output

In file included from test.cpp:1:
spimpl.h: In instantiation of 'bool spimpl::operator<(const unique_impl_ptr<T, D>&, const unique_impl_ptr<T2, D2>&) [with T1 = int; D1 = void (*)(int*); T2 = int; D2 = void (*)(int*)]':
test.cpp:7:13:   required from here
spimpl.h:274:31: error: no match for call to '(std::less<int*>) (spimpl::unique_impl_ptr<int, void (*)(int*)>::const_pointer, spimpl::unique_impl_ptr<int, void (*)(int*)>::const_pointer)'
  274 |         return std::less<CT>()(l.get(), r.get());
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/13.2/bits/unique_ptr.h:37,
                 from /usr/include/c++/13.2/memory:78,
                 from spimpl.h:31:
/usr/include/c++/13.2/bits/stl_function.h:451:7: note: candidate: 'constexpr bool std::less<_Tp*>::operator()(_Tp*, _Tp*) const [with _Tp = int]' (near match)
  451 |       operator()(_Tp* __x, _Tp* __y) const _GLIBCXX_NOTHROW
      |       ^~~~~~~~
/usr/include/c++/13.2/bits/stl_function.h:451:7: note:   conversion of argument 2 would be ill-formed:
spimpl.h:274:46: error: invalid conversion from 'spimpl::unique_impl_ptr<int, void (*)(int*)>::const_pointer' {aka 'const int*'} to 'int*' [-fpermissive]
  274 |         return std::less<CT>()(l.get(), r.get());
      |                                         ~~~~~^~
      |                                              |
      |                                              spimpl::unique_impl_ptr<int, void (*)(int*)>::const_pointer {aka const int*}

clang error output

test.cpp:7:11: warning: relational comparison result unused [-Wunused-comparison]
    7 |     first < second;
      |     ~~~~~~^~~~~~~~
In file included from test.cpp:1:
./spimpl.h:274:16: error: no matching function for call to object of type 'std::less<CT>' (aka 'less<int *>')
  274 |         return std::less<CT>()(l.get(), r.get());
      |                ^~~~~~~~~~~~~~~
test.cpp:7:11: note: in instantiation of function template specialization 'spimpl::operator<<int, void (*)(int *), int, void (*)(int *)>' requested here
    7 |     first < second;
      |           ^
/usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/13.2/../../../../include/c++/13.2/bits/stl_function.h:451:7: note: candidate function not viable: 1st argument ('const_pointer' (aka 'const int *')) would lose const qualifier
  451 |       operator()(_Tp* __x, _Tp* __y) const _GLIBCXX_NOTHROW
      |       ^          ~~~~~~~~
1 warning and 1 error generated.

A non-const std::less instantiation cannot be used for const data.
meator added a commit to meator/samples that referenced this pull request Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant