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

Add const qualifier to proxy::invoke() #28

Merged
merged 2 commits into from
Sep 15, 2022
Merged

Add const qualifier to proxy::invoke() #28

merged 2 commits into from
Sep 15, 2022

Conversation

mingxwa
Copy link
Collaborator

@mingxwa mingxwa commented Sep 15, 2022

No description provided.

@mingxwa mingxwa merged commit 0b5ecfa into main Sep 15, 2022
@mingxwa mingxwa deleted the users/mingxwa/r9 branch September 15, 2022 13:42
@@ -477,10 +478,10 @@ class sbo_ptr {
sbo_ptr(sbo_ptr&&) noexcept(std::is_nothrow_move_constructible_v<T>)
= default;

T& operator*() { return value_; }
T& operator*() const { return value_; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: The one potential downside I see to this is here we have 'physical' const but not 'logical' const as we're returning a non-const reference by calling a const function (meaning you can then mutate the internals of the returned object after calling a const function). I've seen this referred to in the past as 'const broken' (see https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/const.md#classes-of-const-in_correctness for more details). Might it be possible to provide overloads for both const and non-const versions like so...

const T& operator*() const { return value_; }
T& operator*() { return value_; }

If this is purely an implementation detail it might be fine but I just wanted to mention it incase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pr0g

The one potential downside I see to this is here we have 'physical' const but not 'logical' const as we're returning a non-const reference by calling a const function (meaning you can then mutate the internals of the returned object after calling a const function).

In this case, value_ is considered as an internal storage of the pointee (as sbo_ptr implies "small buffer optimization"). There are many similar practices in various implementations of STL. For example, in MSVC implementation of std::function, getting the underlying object is always const, regardless of whether it points to an internal storage. The reason MSVC implementation of std::function does not use mutable is that it stores an addition pointer inside the object, which is not an optimal memory layout and makes it bigger than proxy when the buffer size for SBO is the same.

Might it be possible to provide overloads for both const and non-const versions like so...

const T& operator*() const { return value_; }
T& operator*() { return value_; }
>> ```

No. Doing this will let proxy choose the first overload. Users will not be able to mutate the pointee created from make_proxy when SBO is enabled.

If this is purely an implementation detail it might be fine but I just wanted to mention it incase.

Well noted! Thank you for the comments! Although it is an implementation detail, we still want to keep it as neat as we could😀. Please let us know if there is any further consideration!

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.

3 participants