buzz-lang / BittyBuzz

BittyBuzz is an implementation of Buzz for microcrontrollers.
MIT License
8 stars 7 forks source link

Feat/add table functions #24

Closed xgroleau closed 3 years ago

xgroleau commented 3 years ago

Added global functions for tables manipulation to solve #21. Can now use size, foreach, filter, map and reduce function, matching the Buzz API.

While it's similar to what was done in the neighors functions, instead of pushing the table/accumulator value to the stack and then fetching it, I pass the heap value via the params void pointer.

Comments are more than welcomed.

xgroleau commented 3 years ago

I just realized I may have a bug, I may don't need to pop the self table in the foreach entry function. I'll test a bit more and fix it today or tomorrow.

xgroleau commented 3 years ago

It seems I do need to pop since a nil value is returned. So popping the unused returned value is required, but I am not 100% sure if this is what's happening or I am misdiagnosing. This may be a bug in the neighbors foreach.

beltrame commented 3 years ago

Thank you for the work. Would it be possible to put a test showing the issue and describe the bug that you are talking about?

xgroleau commented 3 years ago

It's a bit hard to make an explicit test for it since it happens in another table segment, but here what I mean.

When running the testneighbors at the foreach test. When the neighbor_foreach_fun is called:

At line 203 the stackptr is at 2 But at line 211 the stackptr is at 3, so an item is not popped after the call

image

Is this the desired behavior to have one item more after the call?

beltrame commented 3 years ago

Yes, you should always have a pop after a closure_call, that's intended. You always have a return value.

xgroleau commented 3 years ago

That's what I though while writing it for normal tables. This means there is a bug in the neighbors foreach, a pop is missing here. I can make another PR or simply add it in this one, whichever you prefer.

beltrame commented 3 years ago

You are correct, that is a bug. I think you can safely add it here.

beltrame commented 3 years ago

Thanks!