Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 1 addition & 32 deletions MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions exploration/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,12 @@ build_test(
name = "traits_and_concepts_build_test",
targets = ["traits_and_concepts"],
)

cc_test(
name = "reference_wrappers_test",
srcs = ["reference_wrappers_test.cc"],
deps = [
"//:indirect",
"@com_google_googletest//:gtest_main",
],
)
82 changes: 82 additions & 0 deletions exploration/reference_wrappers_test.cc
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??
Copy link

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 an operator T& does not make it safer. One can argue that a hardened, narrow contract operator T& is safer. When having such an operator on indirect<T> itself, it just means "the step to reach an lvalue of the object of T when the storage is managed by indirect<T>" has a narrow contract.

Copy link
Owner Author

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.

Copy link

Choose a reason for hiding this comment

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

So "reference_wrapper is non-owning and has no null or valueless state" -- not right; reference_wrapper does have a valueless state, it's just that code cannot observe this state.

Imagine this world: the int object x that holds 42 has a narrow contract on evaluation. The contract is violated if you "move out" the value 42 and then bind it to int& -- isn't that just an operator int&() on int? But wait, what if all I want to do is to assign a new value with x = 7? It won't a problem if that x is in fact indirect<int>, because that expression calls indirect<int>::operator=(int&&).

Copy link

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...

Copy link
Owner Author

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.

Copy link

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 for indirect<T>::operator T&() has always been "support incomplete type T as drop-in replacement for T". It was P3902R1 saying that "reference_wrapper can have that because [...], it would be a safety hole in indirect" and I was saying "it's quite the opposite, reference_wrapper's operator T&() has inevitable safety issues, but indirect<T>'s operator T&() is free of that and would add extra safety to T."


// 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
Loading