Skip to content

Commit ffc14ac

Browse files
Ericson2314Ericson2314
authored
Merge pull request #13983 from xokdvium/bindings-fixes
libexpr: Make Bindings::iterator a proper strong type instead of pointer
2 parents b974b7d + ddabd94 commit ffc14ac

File tree

4 files changed

+84
-41
lines changed

4 files changed

+84
-41
lines changed

‎src/libexpr-tests/primops.cc‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -195,18 +195,18 @@ TEST_F(PrimOpTest, unsafeGetAttrPos)
195195
auto v = eval(expr);
196196
ASSERT_THAT(v, IsAttrsOfSize(3));
197197

198-
auto file = v.attrs()->find(createSymbol("file"));
198+
auto file = v.attrs()->get(createSymbol("file"));
199199
ASSERT_NE(file, nullptr);
200200
ASSERT_THAT(*file->value, IsString());
201201
auto s = baseNameOf(file->value->string_view());
202202
ASSERT_EQ(s, "foo.nix");
203203

204-
auto line = v.attrs()->find(createSymbol("line"));
204+
auto line = v.attrs()->get(createSymbol("line"));
205205
ASSERT_NE(line, nullptr);
206206
state.forceValue(*line->value, noPos);
207207
ASSERT_THAT(*line->value, IsIntEq(4));
208208

209-
auto column = v.attrs()->find(createSymbol("column"));
209+
auto column = v.attrs()->get(createSymbol("column"));
210210
ASSERT_NE(column, nullptr);
211211
state.forceValue(*column->value, noPos);
212212
ASSERT_THAT(*column->value, IsIntEq(3));
@@ -246,7 +246,7 @@ TEST_F(PrimOpTest, removeAttrsRetains)
246246
{
247247
auto v = eval("builtins.removeAttrs { x = 1; y = 2; } [\"x\"]");
248248
ASSERT_THAT(v, IsAttrsOfSize(1));
249-
ASSERT_NE(v.attrs()->find(createSymbol("y")), nullptr);
249+
ASSERT_NE(v.attrs()->get(createSymbol("y")), nullptr);
250250
}
251251

252252
TEST_F(PrimOpTest, listToAttrsEmptyList)
@@ -266,7 +266,7 @@ TEST_F(PrimOpTest, listToAttrs)
266266
{
267267
auto v = eval("builtins.listToAttrs [ { name = \"key\"; value = 123; } ]");
268268
ASSERT_THAT(v, IsAttrsOfSize(1));
269-
auto key = v.attrs()->find(createSymbol("key"));
269+
auto key = v.attrs()->get(createSymbol("key"));
270270
ASSERT_NE(key, nullptr);
271271
ASSERT_THAT(*key->value, IsIntEq(123));
272272
}
@@ -275,7 +275,7 @@ TEST_F(PrimOpTest, intersectAttrs)
275275
{
276276
auto v = eval("builtins.intersectAttrs { a = 1; b = 2; } { b = 3; c = 4; }");
277277
ASSERT_THAT(v, IsAttrsOfSize(1));
278-
auto b = v.attrs()->find(createSymbol("b"));
278+
auto b = v.attrs()->get(createSymbol("b"));
279279
ASSERT_NE(b, nullptr);
280280
ASSERT_THAT(*b->value, IsIntEq(3));
281281
}
@@ -293,11 +293,11 @@ TEST_F(PrimOpTest, functionArgs)
293293
auto v = eval("builtins.functionArgs ({ x, y ? 123}: 1)");
294294
ASSERT_THAT(v, IsAttrsOfSize(2));
295295

296-
auto x = v.attrs()->find(createSymbol("x"));
296+
auto x = v.attrs()->get(createSymbol("x"));
297297
ASSERT_NE(x, nullptr);
298298
ASSERT_THAT(*x->value, IsFalse());
299299

300-
auto y = v.attrs()->find(createSymbol("y"));
300+
auto y = v.attrs()->get(createSymbol("y"));
301301
ASSERT_NE(y, nullptr);
302302
ASSERT_THAT(*y->value, IsTrue());
303303
}
@@ -307,13 +307,13 @@ TEST_F(PrimOpTest, mapAttrs)
307307
auto v = eval("builtins.mapAttrs (name: value: value * 10) { a = 1; b = 2; }");
308308
ASSERT_THAT(v, IsAttrsOfSize(2));
309309

310-
auto a = v.attrs()->find(createSymbol("a"));
310+
auto a = v.attrs()->get(createSymbol("a"));
311311
ASSERT_NE(a, nullptr);
312312
ASSERT_THAT(*a->value, IsThunk());
313313
state.forceValue(*a->value, noPos);
314314
ASSERT_THAT(*a->value, IsIntEq(10));
315315

316-
auto b = v.attrs()->find(createSymbol("b"));
316+
auto b = v.attrs()->get(createSymbol("b"));
317317
ASSERT_NE(b, nullptr);
318318
ASSERT_THAT(*b->value, IsThunk());
319319
state.forceValue(*b->value, noPos);
@@ -839,11 +839,11 @@ TEST_P(ParseDrvNamePrimOpTest, parseDrvName)
839839
auto v = eval(expr);
840840
ASSERT_THAT(v, IsAttrsOfSize(2));
841841

842-
auto name = v.attrs()->find(createSymbol("name"));
842+
auto name = v.attrs()->get(createSymbol("name"));
843843
ASSERT_TRUE(name);
844844
ASSERT_THAT(*name->value, IsStringEq(expectedName));
845845

846-
auto version = v.attrs()->find(createSymbol("version"));
846+
auto version = v.attrs()->get(createSymbol("version"));
847847
ASSERT_TRUE(version);
848848
ASSERT_THAT(*version->value, IsStringEq(expectedVersion));
849849
}

‎src/libexpr-tests/trivial.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,11 @@ TEST_F(TrivialExpressionTest, updateAttrs)
7575
{
7676
auto v = eval("{ a = 1; } // { b = 2; a = 3; }");
7777
ASSERT_THAT(v, IsAttrsOfSize(2));
78-
auto a = v.attrs()->find(createSymbol("a"));
78+
auto a = v.attrs()->get(createSymbol("a"));
7979
ASSERT_NE(a, nullptr);
8080
ASSERT_THAT(*a->value, IsIntEq(3));
8181

82-
auto b = v.attrs()->find(createSymbol("b"));
82+
auto b = v.attrs()->get(createSymbol("b"));
8383
ASSERT_NE(b, nullptr);
8484
ASSERT_THAT(*b->value, IsIntEq(2));
8585
}
@@ -176,19 +176,19 @@ TEST_P(AttrSetMergeTrvialExpressionTest, attrsetMergeLazy)
176176
auto v = eval(expr);
177177
ASSERT_THAT(v, IsAttrsOfSize(1));
178178

179-
auto a = v.attrs()->find(createSymbol("a"));
179+
auto a = v.attrs()->get(createSymbol("a"));
180180
ASSERT_NE(a, nullptr);
181181

182182
ASSERT_THAT(*a->value, IsThunk());
183183
state.forceValue(*a->value, noPos);
184184

185185
ASSERT_THAT(*a->value, IsAttrsOfSize(2));
186186

187-
auto b = a->value->attrs()->find(createSymbol("b"));
187+
auto b = a->value->attrs()->get(createSymbol("b"));
188188
ASSERT_NE(b, nullptr);
189189
ASSERT_THAT(*b->value, IsIntEq(1));
190190

191-
auto c = a->value->attrs()->find(createSymbol("c"));
191+
auto c = a->value->attrs()->get(createSymbol("c"));
192192
ASSERT_NE(c, nullptr);
193193
ASSERT_THAT(*c->value, IsIntEq(2));
194194
}
@@ -330,7 +330,7 @@ TEST_F(TrivialExpressionTest, bindOr)
330330
{
331331
auto v = eval("{ or = 1; }");
332332
ASSERT_THAT(v, IsAttrsOfSize(1));
333-
auto b = v.attrs()->find(createSymbol("or"));
333+
auto b = v.attrs()->get(createSymbol("or"));
334334
ASSERT_NE(b, nullptr);
335335
ASSERT_THAT(*b->value, IsIntEq(1));
336336
}

‎src/libexpr/attr-set.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ Value & BindingsBuilder::alloc(std::string_view name, PosIdx pos)
3535

3636
void Bindings::sort()
3737
{
38-
if (size_)
39-
std::sort(begin(), end());
38+
std::sort(attrs, attrs + size_);
4039
}
4140

4241
Value & Value::mkAttrs(BindingsBuilder & bindings)

‎src/libexpr/include/nix/expr/attr-set.hh‎

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include <algorithm>
88
#include <functional>
9+
#include <concepts>
910

1011
namespace nix {
1112

@@ -81,9 +82,55 @@ public:
8182
return !size_;
8283
}
8384

84-
typedef Attr * iterator;
85-
86-
typedef const Attr * const_iterator;
85+
class iterator
86+
{
87+
public:
88+
using value_type = Attr;
89+
using pointer = const value_type *;
90+
using reference = const value_type &;
91+
using difference_type = std::ptrdiff_t;
92+
using iterator_category = std::forward_iterator_tag;
93+
94+
friend class Bindings;
95+
96+
private:
97+
pointer ptr = nullptr;
98+
99+
explicit iterator(pointer ptr)
100+
: ptr(ptr)
101+
{
102+
}
103+
104+
public:
105+
iterator() = default;
106+
107+
reference operator*() const
108+
{
109+
return *ptr;
110+
}
111+
112+
const value_type * operator->() const
113+
{
114+
return ptr;
115+
}
116+
117+
iterator & operator++()
118+
{
119+
++ptr;
120+
return *this;
121+
}
122+
123+
iterator operator++(int)
124+
{
125+
pointer tmp = ptr;
126+
++*this;
127+
return iterator(tmp);
128+
}
129+
130+
bool operator==(const iterator & rhs) const = default;
131+
};
132+
133+
using const_iterator = iterator;
87134

88135
void push_back(const Attr & attr)
89136
{
@@ -93,39 +140,33 @@ public:
93140
const_iterator find(Symbol name) const
94141
{
95142
Attr key(name, 0);
96-
const_iterator i = std::lower_bound(begin(), end(), key);
97-
if (i != end() && i->name == name)
98-
return i;
143+
auto first = attrs;
144+
auto last = attrs + size_;
145+
const Attr * i = std::lower_bound(first, last, key);
146+
if (i != last && i->name == name)
147+
return const_iterator{i};
99148
return end();
100149
}
101150

102151
const Attr * get(Symbol name) const
103152
{
104153
Attr key(name, 0);
105-
const_iterator i = std::lower_bound(begin(), end(), key);
106-
if (i != end() && i->name == name)
107-
return &*i;
154+
auto first = attrs;
155+
auto last = attrs + size_;
156+
const Attr * i = std::lower_bound(first, last, key);
157+
if (i != last && i->name == name)
158+
return i;
108159
return nullptr;
109160
}
110161

111-
iterator begin()
112-
{
113-
return &attrs[0];
114-
}
115-
116-
iterator end()
117-
{
118-
return &attrs[size_];
119-
}
120-
121162
const_iterator begin() const
122163
{
123-
return &attrs[0];
164+
return const_iterator(attrs);
124165
}
125166

126167
const_iterator end() const
127168
{
128-
return &attrs[size_];
169+
return const_iterator(attrs + size_);
129170
}
130171

131172
Attr & operator[](size_t pos)
@@ -159,6 +200,9 @@ public:
159200
friend class EvalState;
160201
};
161202

203+
static_assert(std::forward_iterator<Bindings::iterator>);
204+
static_assert(std::ranges::forward_range<Bindings>);
205+
162206
/**
163207
* A wrapper around Bindings that ensures that its always in sorted
164208
* order at the end. The only way to consume a BindingsBuilder is to

0 commit comments

Comments
 (0)