Skip to content
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.

Sign up for GitHub

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

Sequence equality #249

Open
wants to merge 60 commits into
base: main
Choose a base branch
from
Open

Sequence equality #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
1fc35d5
wip range equality
frenchy64 Feb 7, 2025
bbc06d0
wip
frenchy64 Feb 7, 2025
a059c68
test core::equal
frenchy64 Feb 7, 2025
701d79f
wip
frenchy64 Feb 7, 2025
8a1472e
[skip ci]
frenchy64 Feb 7, 2025
cbd354f
[skip ci] rm
frenchy64 Feb 7, 2025
3727186
[skip ci]
frenchy64 Feb 7, 2025
ddd82ac
tests
frenchy64 Feb 7, 2025
c2da550
wip
frenchy64 Feb 7, 2025
c035ee8
[skip ci]
frenchy64 Feb 7, 2025
4dc1d92
[skip ci]
frenchy64 Feb 7, 2025
b6d8889
wip
frenchy64 Feb 7, 2025
408ed65
wip
frenchy64 Feb 7, 2025
4529d26
fix
frenchy64 Feb 7, 2025
c227006
[skip ci]
frenchy64 Feb 7, 2025
f1d6e2c
fix
frenchy64 Feb 10, 2025
ca229f8
[skip ci]
frenchy64 Feb 10, 2025
3afba55
wip
frenchy64 Feb 10, 2025
c5f2e33
[skip ci]
frenchy64 Feb 10, 2025
2dbbca6
fmt
frenchy64 Feb 10, 2025
d081d74
Merge branch 'main' into sequence-equality
frenchy64 Feb 10, 2025
4819593
Merge branch 'main' into sequence-equality
frenchy64 Feb 13, 2025
fe6f1dd
assert
frenchy64 Feb 13, 2025
2deb82d
Merge branch 'main' into sequence-equality
frenchy64 Feb 22, 2025
c36ca8d
[skip ci]
frenchy64 Feb 22, 2025
acb06a4
wip
frenchy64 Feb 22, 2025
4a1620f
[skip ci]
frenchy64 Feb 22, 2025
b3ffc63
[skip ci]
frenchy64 Feb 22, 2025
8bf2c34
[skip ci] undo
frenchy64 Feb 22, 2025
6b760d1
[skip ci]
frenchy64 Feb 22, 2025
0bbcbc1
[skip ci]
frenchy64 Feb 22, 2025
5a08204
[skip ci]
frenchy64 Feb 22, 2025
e9c761f
[skip ci]
frenchy64 Feb 22, 2025
a695a90
[skip ci]
frenchy64 Feb 22, 2025
31216fc
[skip ci]
frenchy64 Feb 22, 2025
e8fc0b5
[skip ci]
frenchy64 Feb 22, 2025
6e26023
[skip ci]
frenchy64 Feb 22, 2025
02d217d
[skip ci]
frenchy64 Feb 22, 2025
4979e26
[skip ci]
frenchy64 Feb 22, 2025
5e1e9c3
[skip ci]
frenchy64 Feb 22, 2025
fc1ff00
[skip ci]
frenchy64 Feb 22, 2025
6a1919f
[skip ci]
frenchy64 Feb 22, 2025
10f4798
[skip ci]
frenchy64 Feb 22, 2025
bdecaf0
[skip ci]
frenchy64 Feb 22, 2025
2dc83fe
wip
frenchy64 Feb 22, 2025
99728fa
[skip ci] fmt
frenchy64 Feb 22, 2025
0b8c535
wip
frenchy64 Feb 22, 2025
dd3649e
wip
frenchy64 Feb 22, 2025
674a8e9
[skip ci] revert equal null check
frenchy64 Feb 22, 2025
d754b74
[skip ci] fix first half of chunked cons
frenchy64 Feb 22, 2025
6132099
[skip ci] add bang to next-in-place, fix comments
frenchy64 Feb 22, 2025
7c16d3a
[skip ci] assert
frenchy64 Feb 23, 2025
d9d6281
asserts
frenchy64 Feb 23, 2025
e562511
[skip ci] -else
frenchy64 Feb 23, 2025
0d27f83
add sequence_equal tests
frenchy64 Feb 23, 2025
64a74a2
apply-to
frenchy64 Feb 23, 2025
eee2c29
[skip ci]
frenchy64 Feb 23, 2025
cea8422
tests
frenchy64 Feb 23, 2025
2e08987
fmt
frenchy64 Feb 23, 2025
35a8643
wip
frenchy64 Feb 23, 2025
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
7 changes: 7 additions & 0 deletions compiler+runtime/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,11 +449,18 @@ if(jank_tests)
test/cpp/jank/read/lex.cpp
test/cpp/jank/read/parse.cpp
test/cpp/jank/analyze/box.cpp
test/cpp/jank/runtime/behavior/callable.cpp
test/cpp/jank/runtime/core.cpp
test/cpp/jank/runtime/core/seq.cpp
test/cpp/jank/runtime/detail/native_persistent_list.cpp
test/cpp/jank/runtime/obj/persistent_string.cpp
test/cpp/jank/runtime/obj/ratio.cpp
test/cpp/jank/runtime/obj/persistent_list.cpp
test/cpp/jank/runtime/obj/persistent_string.cpp
test/cpp/jank/runtime/obj/persistent_vector.cpp
test/cpp/jank/runtime/obj/range.cpp
test/cpp/jank/runtime/obj/integer_range.cpp
test/cpp/jank/runtime/obj/repeat.cpp
test/cpp/jank/jit/processor.cpp
)
add_executable(jank::test_exe ALIAS jank_test_exe)
Expand Down
1 change: 1 addition & 0 deletions compiler+runtime/include/cpp/jank/runtime/obj/integer_range.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace jank::runtime::obj

using bounds_check_t = native_bool (*)(integer_ptr, integer_ptr);

/* Constructors are only to be used within integer_range.cpp. Prefer integer_range::create. */
integer_range() = default;
integer_range(integer_range &&) noexcept = default;
integer_range(integer_range const &) = default;
Expand Down
1 change: 1 addition & 0 deletions compiler+runtime/include/cpp/jank/runtime/obj/range.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace jank::runtime::obj

using bounds_check_t = native_bool (*)(object_ptr, object_ptr);

/* Constructors are only to be used within range.cpp. Prefer range::create. */
range() = default;
range(range &&) noexcept = default;
range(range const &) = default;
Expand Down
5 changes: 3 additions & 2 deletions compiler+runtime/include/cpp/jank/runtime/obj/symbol.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,10 @@ namespace std
{
return !rhs;
}
else if(!rhs)
if(!rhs)
{
return !lhs;
assert(lhs);
return false;
}
return lhs->equal(*rhs);
}
Expand Down
5 changes: 3 additions & 2 deletions compiler+runtime/include/cpp/jank/runtime/var.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,10 @@ namespace std
{
return !rhs;
}
else if(!rhs)
if(!rhs)
{
return !lhs;
assert(lhs);
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed in slack, none of these assertions on native boxes are needed. It has its own.

return false;
}
return lhs->equal(*rhs);
}
Expand Down
6 changes: 4 additions & 2 deletions compiler+runtime/src/cpp/clojure/core_native.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,8 +798,10 @@ jank_object_ptr jank_load_clojure_core_native()
{
auto const fn(
make_box<obj::jit_function>(behavior::callable::build_arity_flags(2, false, false)));
fn->arity_1 = [](object * const val) -> object * { return repeat(val); };
fn->arity_2 = [](object * const n, object * const val) -> object * { return repeat(n, val); };
fn->arity_1 = [](object * const val) -> object * { return obj::repeat::create(val); };
fn->arity_2 = [](object * const n, object * const val) -> object * {
return obj::repeat::create(n, val);
};
intern_fn_obj("repeat", fn);
}

Expand Down
4 changes: 1 addition & 3 deletions compiler+runtime/src/cpp/jank/read/parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@ namespace jank::read::parse
auto const jank_keyword(__rt_ctx->intern_keyword("", "jank").expect_ok());
auto const default_keyword(__rt_ctx->intern_keyword("", "default").expect_ok());

for(auto it(list->fresh_seq()); it != nullptr;)
for(auto it(list->fresh_seq()); it != nullptr; it = next_in_place(next_in_place(it)))
{
auto const kw(it->first());
/* We take the first match, checking for :jank first. If there are duplicates, it doesn't
Expand Down Expand Up @@ -829,8 +829,6 @@ namespace jank::read::parse
return object_source_info{ next_in_place(it)->first(), start_token, list_end };
}
}

it = next_in_place(next_in_place(it));
}

return ok(none);
Expand Down
112 changes: 56 additions & 56 deletions compiler+runtime/src/cpp/jank/runtime/behavior/callable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ namespace jank::runtime
{
return visit_seqable(
[=](auto const typed_args) -> object_ptr {
auto const s(typed_args->fresh_seq());
auto s(typed_args->fresh_seq());
auto const length(sequence_length(s, max_params + 1));
switch(length)
{
Expand All @@ -878,88 +878,88 @@ namespace jank::runtime
case 1:
return dynamic_call(source, s->first());
case 2:
return dynamic_call(source, s->first(), s->next_in_place()->first());
return dynamic_call(source, s->first(), (s = s->next_in_place())->first());
Copy link
Member

Choose a reason for hiding this comment

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

I have a local TODO that notes that persistent_list_sequence shouldn't exist, since persistent_list itself is a sequence.

This change is a big red flag, since this is an extremely hot code path. I don't want to impact its perf for this one case which needs to change anyway.

If you want to add a comment here noting that it doesn't work for persistent_list, that's ok. If you want to remove persistent_list_sequence, that's also ok, but let's do it in a separate PR.

Either way, let's please undo this file's changes.

case 3:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 4:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 5:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 6:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 7:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 8:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 9:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
case 10:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first());
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first());
default:
return dynamic_call(source,
s->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
s->next_in_place()->first(),
obj::persistent_list::create(next_in_place(s)));
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
(s = s->next_in_place())->first(),
obj::persistent_list::create(s = next_in_place(s)));
}
},
args);
Expand Down
5 changes: 3 additions & 2 deletions compiler+runtime/src/cpp/jank/runtime/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,10 @@ namespace jank::runtime
{
return !rhs;
}
else if(!rhs)
if(!rhs)
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the else? I find it helps with the logical flow. Also, by having the else if, your assertion is proven by the compiler to be redundant.

{
return !lhs;
assert(lhs);
return false;
}

return visit_object([&](auto const typed_lhs) { return typed_lhs->equal(*rhs); }, lhs);
Expand Down
45 changes: 8 additions & 37 deletions compiler+runtime/src/cpp/jank/runtime/core/seq.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,21 +478,13 @@ namespace jank::runtime
{
return make_box<obj::persistent_list>(std::in_place, o);
}
else if constexpr(behavior::conjable_in_place<T>)
{
return typed_s->conj_in_place(o);
}
else if constexpr(behavior::conjable<T>)
{
return typed_s->conj(o);
}
else if constexpr(behavior::seqable<T>)
{
return typed_s->seq()->conj(o);
}
else
{
throw std::runtime_error{ fmt::format("not seqable: {}", typed_s->to_string()) };
throw std::runtime_error{ fmt::format("not conjable: {}", typed_s->to_string()) };
}
},
s);
Expand Down Expand Up @@ -521,11 +513,7 @@ namespace jank::runtime
[&](auto const typed_m) -> object_ptr {
using T = typename decltype(typed_m)::value_type;

if constexpr(behavior::associatively_writable_in_place<T>)
{
return typed_m->assoc_in_place(k, v);
}
else if constexpr(behavior::associatively_writable<T>)
if constexpr(behavior::associatively_writable<T>)
{
return typed_m->assoc(k, v);
}
Expand All @@ -544,11 +532,7 @@ namespace jank::runtime
[&](auto const typed_m) -> object_ptr {
using T = typename decltype(typed_m)::value_type;

if constexpr(behavior::associatively_writable_in_place<T>)
{
return typed_m->dissoc_in_place(k);
}
else if constexpr(behavior::associatively_writable<T>)
if constexpr(behavior::associatively_writable<T>)
{
return typed_m->dissoc(k);
}
Expand Down Expand Up @@ -1025,23 +1009,10 @@ namespace jank::runtime
return visit_seqable(
[](auto const typed_r, auto const typed_l) -> native_bool {
auto r_it(typed_r->fresh_seq());
auto l_it(typed_l->fresh_seq());
if(!r_it)
for(auto l_it(typed_l->fresh_seq()); l_it != nullptr;
l_it = l_it->next_in_place(), r_it = r_it->next_in_place())
{
return l_it == nullptr;
}
if(!l_it)
{
return r_it == nullptr;
}

for(; l_it != nullptr; l_it = l_it->next_in_place(), r_it = r_it->next_in_place())
{
if(!r_it)
{
return false;
}
if(!runtime::equal(l_it->first(), r_it->first()))
if(!r_it || !runtime::equal(l_it->first(), r_it->first()))
{
return false;
}
Expand Down Expand Up @@ -1178,12 +1149,12 @@ namespace jank::runtime

object_ptr repeat(object_ptr const val)
{
return make_box<obj::repeat>(val);
return obj::repeat::create(val);
}

object_ptr repeat(object_ptr const n, object_ptr const val)
{
return make_box<obj::repeat>(n, val);
return obj::repeat::create(n, val);
}

object_ptr sort(object_ptr const coll)
Expand Down
7 changes: 5 additions & 2 deletions compiler+runtime/src/cpp/jank/runtime/obj/chunked_cons.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,15 @@ namespace jank::runtime::obj
for(auto it(fresh_seq()); it != nullptr;
it = runtime::next_in_place(it), seq = runtime::next_in_place(seq))
{
if(seq == nullptr || !runtime::equal(it, seq->first()))
assert(it != nil::nil_const());
assert(seq != nil::nil_const());
Comment on lines +165 to +166
Copy link
Member

Choose a reason for hiding this comment

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

It's impossible for these assertions to be false, proven by the type system. I think you're concerned about the next_in_place(it) call somehow returning an object_ptr. But it could never do that, since it would result in a compiler error.

Now, the history is that next_in_place has been in flux for a year or so and I was making some changes which would allow for it to be optional. That never happened, but it's why we're not using it->next_in_place() here. Ultimately, it's doing the same thing and all of these can be changed back. I just haven't done that yet, since it's a perf change and I haven't been focusing on perf.

Either way, your concerns about this being nil should not be keeping you up. auto it(fresh_seq()); means that we'll have a fully typed it for the currently visited object. We cannot assign an object_ptr to it. C++ won't allow it.

if(seq == nullptr || !runtime::equal(it->first(), seq->first()))
{
return false;
}
}
return true;
assert(seq != nil::nil_const());
return seq == nullptr;
},
[]() { return false; },
&o);
Expand Down
7 changes: 5 additions & 2 deletions compiler+runtime/src/cpp/jank/runtime/obj/cons.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,15 @@ namespace jank::runtime::obj
for(auto it(fresh_seq()); it != nullptr;
it = runtime::next_in_place(it), seq = runtime::next_in_place(seq))
{
if(seq == nullptr || !runtime::equal(it, seq->first()))
assert(it != nil::nil_const());
assert(seq != nil::nil_const());
if(seq == nullptr || !runtime::equal(it->first(), seq->first()))
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good fix. We'd need the same thing for chunked_cons above, though. I wonder where else this cropped up. I have a TODO for sequence equality having a lot of copy pasta.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd need the same thing for chunked_cons above, though.

Done.

I wonder where else this cropped up.

Several places. I think I reverted some of them accidentally when experimenting with replace nullptr with nil_const(), I'll have another look.

I have a TODO for sequence equality having a lot of copy pasta.

I wasn't sure how to tackle it, but I can at least write more tests so then the abstraction can be validated easily.

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about removing support for nullptr yet. Certainly not in this PR, which already has a lot going on.

Copy link
Member

@jeaye jeaye Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't sure how to tackle it, but I can at least write more tests so then the abstraction can be validated easily.

I have some ideas for how to do it. The tests are very welcome, though.

{
return false;
}
}
return true;
assert(seq != nil::nil_const());
return seq == nullptr;
},
[]() { return false; },
&o);
Expand Down
4 changes: 2 additions & 2 deletions compiler+runtime/src/cpp/jank/runtime/obj/detail/base_persistent_map_sequence.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ namespace jank::runtime::obj::detail
for(auto it(fresh_seq()); it != nullptr;
it = it->next_in_place(), seq = seq->next_in_place())
{
if(seq == nullptr || !runtime::equal(it, seq->first()))
if(seq == nullptr || !runtime::equal(it->first(), seq->first()))
{
return false;
}
}
return true;
return seq == nullptr;
},
[]() { return false; },
&o);
Expand Down
Loading