-
Notifications
You must be signed in to change notification settings - Fork 15
Add experiment with reference-wrappers #609
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
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
10cd630
Add experiment with reference-wrappers
jbcoe 0d3beb7
Add some comments
jbcoe c2bee5d
Add exploratory comments after running with ASAN
jbcoe 16d248e
use implicit reference conversion not get()
jbcoe 76d7766
Address Copilot review comments
jbcoe 697c43f
Add Zhihao's comment
jbcoe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| #include <gtest/gtest.h> | ||
|
|
||
| #include <functional> | ||
|
|
||
| #include "indirect.h" | ||
|
|
||
| namespace { | ||
|
|
||
| TEST(IndirectExploration, ReferenceWrapperAndSwap) { | ||
| // Given two dynamically allocated values managed with `indirect`. | ||
| auto a = xyz::indirect(3); | ||
| auto b = xyz::indirect(4); | ||
|
|
||
| // Values are as expected. | ||
| EXPECT_EQ(*a, 3); | ||
| EXPECT_EQ(*b, 4); | ||
|
|
||
| // Given references to the values. | ||
| auto ar = std::ref(*a); | ||
| auto br = std::ref(*b); | ||
|
|
||
| // Values accessed through the references are as expected. | ||
| EXPECT_EQ(ar, 3); | ||
| EXPECT_EQ(br, 4); | ||
|
|
||
| // When we swap the two indirect values. | ||
| swap(a, b); | ||
| // Then the values themselves have been swapped. | ||
| EXPECT_EQ(*a, 4); | ||
| EXPECT_EQ(*b, 3); | ||
|
|
||
| // But the reference values refer to the original values. | ||
| EXPECT_EQ(ar, 3); | ||
| EXPECT_EQ(br, 4); | ||
|
|
||
| // When we swap the two reference wrappers. | ||
| swap(ar, br); | ||
|
|
||
| // Then the values accessed through references have been swapped. | ||
| EXPECT_EQ(ar, 4); | ||
| EXPECT_EQ(br, 3); | ||
| } | ||
|
|
||
| TEST(IndirectExploration, ReferenceWrapperAndMove) { | ||
| // Given two dynamically allocated values managed with `indirect`. | ||
| auto a = xyz::indirect(3); | ||
| auto b = xyz::indirect(4); | ||
|
|
||
| // Values are as expected. | ||
| EXPECT_EQ(*a, 3); | ||
| EXPECT_EQ(*b, 4); | ||
|
|
||
| // Given references to the values. | ||
| auto ar = std::ref(*a); | ||
| auto br = std::ref(*b); | ||
|
|
||
| // Values accessed through the references are as expected. | ||
| EXPECT_EQ(ar, 3); | ||
| EXPECT_EQ(br, 4); | ||
|
|
||
| // When we move values and references. | ||
| a = std::move(b); // this renders `b` valueless. | ||
|
|
||
| // Note: At this point ar is a reference to a value which no longer exists. | ||
| // EXPECT_EQ(ar, 3); - This would lead to heap-use-after-free when run under | ||
| // ASAN. | ||
|
|
||
| ar = std::move(br); // this does nothing to `br`. | ||
|
|
||
| // The moved-from indirect is valueless. | ||
| EXPECT_TRUE(b.valueless_after_move()); | ||
| // b-ref refers to the value it referred to before the move. | ||
| EXPECT_EQ(br, 4); // observing the lvalue via operator int&(). | ||
| // Perhaps this is surprising?? | ||
|
|
||
| // The `a` indirect and `a` reference-wrapper now refer to the moved-from | ||
| // value. | ||
| EXPECT_EQ(*a, 4); | ||
| EXPECT_EQ(ar, 4); // observing the lvalue via operator int&() | ||
| } | ||
|
|
||
| } // namespace | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and this means that, to
reference_wrapper<T>, the wide contract on anoperator T&does not make it safer. One can argue that a hardened, narrow contractoperator T&is safer. When having such an operator onindirect<T>itself, it just means "the step to reach an lvalue of the object ofTwhen the storage is managed byindirect<T>" has a narrow contract.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I'd agree that reference-wrapper is unsafe here, but that's expected. I'm not sure that I see the point you're making clearly yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So "
reference_wrapperis non-owning and has no null or valueless state" -- not right;reference_wrapperdoes have a valueless state, it's just that code cannot observe this state.Imagine this world: the
intobjectxthat holds42has a narrow contract on evaluation. The contract is violated if you "move out" the value42and then bind it toint&-- isn't that just anoperator int&()onint? But wait, what if all I want to do is to assign a new value withx = 7? It won't a problem if thatxis in factindirect<int>, because that expression callsindirect<int>::operator=(int&&).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
indirect<T>::operator T&()with a narrow contract is totally safe, extra-safe I'd say. It gets involved in the right contexts, and is not involved when not in the right context...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what I can do about this: "reference_wrapper does have a valueless state, it's just that code cannot observe this state."
In any case the issues we see seem to be issues with reference_wrapper, not indirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sentence is not meant to point out any issue with
indirect. I mean, the whole motivation forindirect<T>::operator T&()has always been "support incomplete typeTas drop-in replacement forT". It was P3902R1 saying that "reference_wrappercan have that because [...], it would be a safety hole inindirect" and I was saying "it's quite the opposite,reference_wrapper'soperator T&()has inevitable safety issues, butindirect<T>'soperator T&()is free of that and would add extra safety toT."