Sequence equality #249
Sequence equality #249
Conversation
| return !lhs; | ||
| return false; |
There was a problem hiding this comment.
Why change this? Right now, it would treat two null object ptrs as equal. This would also carry over to us removing nullptr support for boxes, such that two nils would be equal.
There was a problem hiding this comment.
Thanks, I missed that box overloaded !.
There was a problem hiding this comment.
Actually, could you look at this again? At this point we already know !lhs is false, so I think returning either is equivalent.
Would this be another way of writing the test?
if(!lhs != !rhs) return falseEDIT: No, because that is false when both are null.
There was a problem hiding this comment.
Added assertions to point out my reasoning.
| return true; | ||
| return seq == nullptr; |
There was a problem hiding this comment.
I don't think this is correct. In the case where this is a shorter seq than o, we would finish our for loop and then need to check if there is some seq left over.
There was a problem hiding this comment.
In the case where this is a shorter seq than o, we would finish our for loop and then need to check if there is some seq left over.
This is my attempt at adding that check. At this point, we've already called next_in_place(seq), and seq will be nullptr if there's none left over.
There was a problem hiding this comment.
We're talking about the same diff, right?
- return true;
+ return seq == nullptr;
| @@ -77,12 +77,12 @@ 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())) | |||
| if(seq == nullptr || !runtime::equal(it->first(), seq->first())) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Don't worry about removing support for nullptr yet. Certainly not in this PR, which already has a lot going on.
There was a problem hiding this comment.
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.
|
Found and fixed a couple more issues while I scanned everything. Lmk if I should pull them out.
|
Closes #243
Closes #250
Closes #251
Closes #274
It would be nice to replace
nullptrreturns withnilinsequenceablebut it enables important optimizations for calls tosequenceablemember functions. I did not investigate returningoptiontypes.