Skip to content

Commit 1f1ccf7

Browse files
jckingcopybara-github
jcking
authored and
copybara-github
committed
Reimplement ComprehensionSlots to preserve pointer stability
PiperOrigin-RevId: 743227134
1 parent 5d1c5e4 commit 1f1ccf7

7 files changed

Lines changed: 194 additions & 134 deletions

File tree

‎eval/eval/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,10 @@ cc_library(
100100
deps = [
101101
":attribute_trail",
102102
"//common:value",
103+
"@com_google_absl//absl/base:core_headers",
103104
"@com_google_absl//absl/base:no_destructor",
104105
"@com_google_absl//absl/base:nullability",
106+
"@com_google_absl//absl/container:fixed_array",
105107
"@com_google_absl//absl/log:absl_check",
106108
"@com_google_absl//absl/types:optional",
107109
],
@@ -1181,6 +1183,7 @@ cc_library(
11811183
hdrs = ["lazy_init_step.h"],
11821184
deps = [
11831185
":attribute_trail",
1186+
":comprehension_slots",
11841187
":direct_expression_step",
11851188
":evaluator_core",
11861189
":expression_step_base",

‎eval/eval/comprehension_slots.h‎

Lines changed: 87 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -17,30 +17,91 @@
1717

1818
#include <cstddef>
1919
#include <utility>
20-
#include <vector>
2120

21+
#include "absl/base/attributes.h"
2222
#include "absl/base/no_destructor.h"
2323
#include "absl/base/nullability.h"
24+
#include "absl/container/fixed_array.h"
2425
#include "absl/log/absl_check.h"
2526
#include "absl/types/optional.h"
2627
#include "common/value.h"
2728
#include "eval/eval/attribute_trail.h"
2829

2930
namespace google::api::expr::runtime {
3031

32+
class ComprehensionSlot final {
33+
public:
34+
ComprehensionSlot() = default;
35+
ComprehensionSlot(const ComprehensionSlot&) = delete;
36+
ComprehensionSlot(ComprehensionSlot&&) = delete;
37+
ComprehensionSlot& operator=(const ComprehensionSlot&) = delete;
38+
ComprehensionSlot& operator=(ComprehensionSlot&&) = delete;
39+
40+
const cel::Value& value() const ABSL_ATTRIBUTE_LIFETIME_BOUND {
41+
ABSL_DCHECK(Has());
42+
43+
return value_;
44+
}
45+
46+
absl::Nonnull<cel::Value*> mutable_value() ABSL_ATTRIBUTE_LIFETIME_BOUND {
47+
ABSL_DCHECK(Has());
48+
49+
return &value_;
50+
}
51+
52+
const AttributeTrail& attribute() const ABSL_ATTRIBUTE_LIFETIME_BOUND {
53+
ABSL_DCHECK(Has());
54+
55+
return attribute_;
56+
}
57+
58+
absl::Nonnull<AttributeTrail*> mutable_attribute()
59+
ABSL_ATTRIBUTE_LIFETIME_BOUND {
60+
ABSL_DCHECK(Has());
61+
62+
return &attribute_;
63+
}
64+
65+
bool Has() const { return has_; }
66+
67+
void Set() { Set(cel::NullValue(), absl::nullopt); }
68+
69+
template <typename V>
70+
void Set(V&& value) {
71+
Set(std::forward<V>(value), absl::nullopt);
72+
}
73+
74+
template <typename V, typename A>
75+
void Set(V&& value, A&& attribute) {
76+
value_ = std::forward<V>(value);
77+
attribute_ = std::forward<A>(attribute);
78+
has_ = true;
79+
}
80+
81+
void Clear() {
82+
if (has_) {
83+
value_ = cel::NullValue();
84+
attribute_ = absl::nullopt;
85+
has_ = false;
86+
}
87+
}
88+
89+
private:
90+
cel::Value value_;
91+
AttributeTrail attribute_;
92+
bool has_ = false;
93+
};
94+
3195
// Simple manager for comprehension variables.
3296
//
3397
// At plan time, each comprehension variable is assigned a slot by index.
3498
// This is used instead of looking up the variable identifier by name in a
3599
// runtime stack.
36100
//
37101
// Callers must handle range checking.
38-
class ComprehensionSlots {
102+
class ComprehensionSlots final {
39103
public:
40-
struct Slot {
41-
cel::Value value;
42-
AttributeTrail attribute;
43-
};
104+
using Slot = ComprehensionSlot;
44105

45106
// Trivial instance if no slots are needed.
46107
// Trivially thread safe since no effective state.
@@ -49,52 +110,42 @@ class ComprehensionSlots {
49110
return *instance;
50111
}
51112

52-
explicit ComprehensionSlots(size_t size) : size_(size), slots_(size) {}
113+
explicit ComprehensionSlots(size_t size) : slots_(size) {}
53114

54-
// Move only
55115
ComprehensionSlots(const ComprehensionSlots&) = delete;
56116
ComprehensionSlots& operator=(const ComprehensionSlots&) = delete;
57-
ComprehensionSlots(ComprehensionSlots&&) = default;
58-
ComprehensionSlots& operator=(ComprehensionSlots&&) = default;
59-
60-
// Return ptr to slot at index.
61-
// If not set, returns nullptr.
62-
absl::Nullable<Slot*> Get(size_t index) {
63-
ABSL_DCHECK_LT(index, slots_.size());
64-
auto& slot = slots_[index];
65-
if (!slot.has_value()) return nullptr;
66-
return &slot.value();
67-
}
68117

69-
void Reset() {
70-
slots_.clear();
71-
slots_.resize(size_);
72-
}
118+
ComprehensionSlots(ComprehensionSlots&&) = delete;
119+
ComprehensionSlots& operator=(ComprehensionSlots&&) = delete;
73120

74-
void ClearSlot(size_t index) {
75-
ABSL_DCHECK_LT(index, slots_.size());
76-
slots_[index] = absl::nullopt;
121+
absl::Nonnull<Slot*> Get(size_t index) ABSL_ATTRIBUTE_LIFETIME_BOUND {
122+
ABSL_DCHECK_LT(index, size());
123+
124+
return &slots_[index];
77125
}
78126

79-
absl::Nonnull<Slot*> Set(size_t index) {
80-
ABSL_DCHECK_LT(index, slots_.size());
81-
return &slots_[index].emplace();
127+
void Reset() {
128+
for (Slot& slot : slots_) {
129+
slot.Clear();
130+
}
82131
}
83132

84-
void Set(size_t index, cel::Value value) {
85-
Set(index, std::move(value), AttributeTrail());
133+
void ClearSlot(size_t index) { Get(index)->Clear(); }
134+
135+
template <typename V>
136+
void Set(size_t index, V&& value) {
137+
Set(index, std::forward<V>(value), absl::nullopt);
86138
}
87139

88-
void Set(size_t index, cel::Value value, AttributeTrail attribute) {
89-
ABSL_DCHECK_LT(index, slots_.size());
90-
slots_[index] = Slot{std::move(value), std::move(attribute)};
140+
template <typename V, typename A>
141+
void Set(size_t index, V&& value, A&& attribute) {
142+
Get(index)->Set(std::forward<V>(value), std::forward<A>(attribute));
91143
}
92144

93145
size_t size() const { return slots_.size(); }
94146

95147
private:
96-
size_t size_;
97-
std::vector<absl::optional<Slot>> slots_;
148+
absl::FixedArray<ComprehensionSlot, 0> slots_;
98149
};
99150

100151
} // namespace google::api::expr::runtime

‎eval/eval/comprehension_slots_test.cc‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,44 +35,41 @@ using ::testing::Truly;
3535
TEST(ComprehensionSlots, Basic) {
3636
ComprehensionSlots slots(4);
3737

38-
ComprehensionSlots::Slot* unset = slots.Get(0);
39-
EXPECT_EQ(unset, nullptr);
38+
ComprehensionSlots::Slot* slot0 = slots.Get(0);
39+
EXPECT_FALSE(slot0->Has());
4040

4141
slots.Set(0, cel::StringValue("abcd"),
4242
AttributeTrail(Attribute("fake_attr")));
4343

44-
auto* slot0 = slots.Get(0);
45-
ASSERT_TRUE(slot0 != nullptr);
44+
ASSERT_TRUE(slot0->Has());
4645

47-
EXPECT_THAT(slot0->value, Truly([](const Value& v) {
46+
EXPECT_THAT(slot0->value(), Truly([](const Value& v) {
4847
return v.Is<StringValue>() &&
4948
v.GetString().ToString() == "abcd";
5049
}))
5150
<< "value is 'abcd'";
5251

53-
EXPECT_THAT(slot0->attribute.attribute().AsString(),
52+
EXPECT_THAT(slot0->attribute().attribute().AsString(),
5453
IsOkAndHolds("fake_attr"));
5554

5655
slots.ClearSlot(0);
57-
EXPECT_EQ(slots.Get(0), nullptr);
56+
EXPECT_FALSE(slot0->Has());
5857

5958
slots.Set(3, cel::StringValue("abcd"),
6059
AttributeTrail(Attribute("fake_attr")));
6160

6261
auto* slot3 = slots.Get(3);
6362

64-
ASSERT_TRUE(slot3 != nullptr);
65-
EXPECT_THAT(slot3->value, Truly([](const Value& v) {
63+
ASSERT_TRUE(slot3->Has());
64+
EXPECT_THAT(slot3->value(), Truly([](const Value& v) {
6665
return v.Is<StringValue>() &&
6766
v.GetString().ToString() == "abcd";
6867
}))
6968
<< "value is 'abcd'";
7069

7170
slots.Reset();
72-
slot0 = slots.Get(0);
73-
EXPECT_TRUE(slot0 == nullptr);
74-
slot3 = slots.Get(3);
75-
EXPECT_TRUE(slot3 == nullptr);
71+
EXPECT_FALSE(slot0->Has());
72+
EXPECT_FALSE(slot3->Has());
7673
}
7774

7875
} // namespace google::api::expr::runtime

0 commit comments

Comments
 (0)