From 10cd630cb83267ea05121a2510a21a727bf79916 Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 12:54:35 -1000 Subject: [PATCH 1/6] Add experiment with reference-wrappers --- MODULE.bazel.lock | 33 +---------- exploration/BUILD.bazel | 9 +++ exploration/reference_wrappers_test.cc | 76 ++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 32 deletions(-) create mode 100644 exploration/reference_wrappers_test.cc diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 741c4267..b723bfe9 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -152,37 +152,6 @@ }, "selectedYankedVersions": {}, "moduleExtensions": { - "@@apple_support+//crosstool:setup.bzl%apple_cc_configure_extension": { - "general": { - "bzlTransitiveDigest": "E970FlMbwpgJPdPUQzatKh6BMfeE0ZpWABvwshh7Tmg=", - "usagesDigest": "aYRVMk+1OupIp+5hdBlpzT36qgd6ntgSxYTzMLW5K4U=", - "recordedFileInputs": {}, - "recordedDirentsInputs": {}, - "envVariables": {}, - "generatedRepoSpecs": { - "local_config_apple_cc_toolchains": { - "repoRuleId": "@@apple_support+//crosstool:setup.bzl%_apple_cc_autoconf_toolchains", - "attributes": {} - }, - "local_config_apple_cc": { - "repoRuleId": "@@apple_support+//crosstool:setup.bzl%_apple_cc_autoconf", - "attributes": {} - } - }, - "recordedRepoMappingEntries": [ - [ - "apple_support+", - "bazel_tools", - "bazel_tools" - ], - [ - "bazel_tools", - "rules_cc", - "rules_cc+" - ] - ] - } - }, "@@rules_fuzzing+//fuzzing/private:extensions.bzl%non_module_dependencies": { "general": { "bzlTransitiveDigest": "mGiTB79hRNjmeDTQdzkpCHyzXhErMbufeAmySBt7s5s=", @@ -268,7 +237,7 @@ }, "@@rules_kotlin+//src/main/starlark/core/repositories:bzlmod_setup.bzl%rules_kotlin_extensions": { "general": { - "bzlTransitiveDigest": "hUTp2w+RUVdL7ma5esCXZJAFnX7vLbVfLd7FwnQI6bU=", + "bzlTransitiveDigest": "OlvsB0HsvxbR8ZN+J9Vf00X/+WVz/Y/5Xrq2LgcVfdo=", "usagesDigest": "QI2z8ZUR+mqtbwsf2fLqYdJAkPOHdOV+tF2yVAUgRzw=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, diff --git a/exploration/BUILD.bazel b/exploration/BUILD.bazel index 37ae3622..cbc98888 100644 --- a/exploration/BUILD.bazel +++ b/exploration/BUILD.bazel @@ -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", + ], +) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc new file mode 100644 index 00000000..3b5a5933 --- /dev/null +++ b/exploration/reference_wrappers_test.cc @@ -0,0 +1,76 @@ +#include + +#include + +#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.get(), 3); + EXPECT_EQ(br.get(), 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 value refer to the original values. + EXPECT_EQ(ar.get(), 3); + EXPECT_EQ(br.get(), 4); + + // When we swap the two reference wrappers. + swap(ar, br); + + // Then the values accessed through references have been swapped. + EXPECT_EQ(ar.get(), 4); + EXPECT_EQ(br.get(), 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.get(), 3); + EXPECT_EQ(br.get(), 4); + + // When we move values and references. + a = std::move(b); + ar = std::move(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); + + // The `a` indirect and `a` reference-wrapper now refer to the moved-from + // value. + EXPECT_EQ(*a, 4); + EXPECT_EQ(ar, 4); +} + +} // namespace From 0d3beb74e52ddde17633e6d62f5ce3017f40aa03 Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 12:59:31 -1000 Subject: [PATCH 2/6] Add some comments --- exploration/reference_wrappers_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc index 3b5a5933..019bea7e 100644 --- a/exploration/reference_wrappers_test.cc +++ b/exploration/reference_wrappers_test.cc @@ -59,8 +59,8 @@ TEST(IndirectExploration, ReferenceWrapperAndMove) { EXPECT_EQ(br.get(), 4); // When we move values and references. - a = std::move(b); - ar = std::move(br); + a = std::move(b); // this renders `b` valueless. + ar = std::move(br); // this does nothing to `br`. // The moved-from indirect is valueless. EXPECT_TRUE(b.valueless_after_move()); From c2bee5d85da3c784c283fb6ae95442cde5f15962 Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 13:23:35 -1000 Subject: [PATCH 3/6] Add exploratory comments after running with ASAN --- exploration/reference_wrappers_test.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc index 019bea7e..b233e4e2 100644 --- a/exploration/reference_wrappers_test.cc +++ b/exploration/reference_wrappers_test.cc @@ -59,13 +59,18 @@ TEST(IndirectExploration, ReferenceWrapperAndMove) { EXPECT_EQ(br.get(), 4); // When we move values and references. - a = std::move(b); // this renders `b` valueless. + 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); + EXPECT_EQ(br, 4); // Perhaps this is surprising?? // The `a` indirect and `a` reference-wrapper now refer to the moved-from // value. From 16d248eebce1cab331f8bc7acf4f9df8af98fd4a Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 13:26:47 -1000 Subject: [PATCH 4/6] use implicit reference conversion not get() --- exploration/reference_wrappers_test.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc index b233e4e2..81e0ac1a 100644 --- a/exploration/reference_wrappers_test.cc +++ b/exploration/reference_wrappers_test.cc @@ -20,8 +20,8 @@ TEST(IndirectExploration, ReferenceWrapperAndSwap) { auto br = std::ref(*b); // Values accessed through the references are as expected. - EXPECT_EQ(ar.get(), 3); - EXPECT_EQ(br.get(), 4); + EXPECT_EQ(ar, 3); + EXPECT_EQ(br, 4); // When we swap the two indirect values. swap(a, b); @@ -30,15 +30,15 @@ TEST(IndirectExploration, ReferenceWrapperAndSwap) { EXPECT_EQ(*b, 3); // But the reference value refer to the original values. - EXPECT_EQ(ar.get(), 3); - EXPECT_EQ(br.get(), 4); + 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.get(), 4); - EXPECT_EQ(br.get(), 3); + EXPECT_EQ(ar, 4); + EXPECT_EQ(br, 3); } TEST(IndirectExploration, ReferenceWrapperAndMove) { @@ -55,8 +55,8 @@ TEST(IndirectExploration, ReferenceWrapperAndMove) { auto br = std::ref(*b); // Values accessed through the references are as expected. - EXPECT_EQ(ar.get(), 3); - EXPECT_EQ(br.get(), 4); + EXPECT_EQ(ar, 3); + EXPECT_EQ(br, 4); // When we move values and references. a = std::move(b); // this renders `b` valueless. From 76d7766f0d7701f6c5b8a3204d1ae0a75ab03b7f Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 13:29:48 -1000 Subject: [PATCH 5/6] Address Copilot review comments --- exploration/reference_wrappers_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc index 81e0ac1a..1b277e79 100644 --- a/exploration/reference_wrappers_test.cc +++ b/exploration/reference_wrappers_test.cc @@ -29,7 +29,7 @@ TEST(IndirectExploration, ReferenceWrapperAndSwap) { EXPECT_EQ(*a, 4); EXPECT_EQ(*b, 3); - // But the reference value refer to the original values. + // But the reference values refer to the original values. EXPECT_EQ(ar, 3); EXPECT_EQ(br, 4); @@ -75,7 +75,7 @@ TEST(IndirectExploration, ReferenceWrapperAndMove) { // The `a` indirect and `a` reference-wrapper now refer to the moved-from // value. EXPECT_EQ(*a, 4); - EXPECT_EQ(ar, 4); + EXPECT_EQ(ar, 4); // observing the lvalue via operator int&() } } // namespace From 697c43f6cbe78a0b38f520610e1d48cb26b84217 Mon Sep 17 00:00:00 2001 From: "Jonathan B. Coe" Date: Mon, 3 Nov 2025 13:31:26 -1000 Subject: [PATCH 6/6] Add Zhihao's comment --- exploration/reference_wrappers_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/exploration/reference_wrappers_test.cc b/exploration/reference_wrappers_test.cc index 1b277e79..79940edb 100644 --- a/exploration/reference_wrappers_test.cc +++ b/exploration/reference_wrappers_test.cc @@ -70,7 +70,8 @@ TEST(IndirectExploration, ReferenceWrapperAndMove) { // 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); // Perhaps this is surprising?? + 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.