Assembler simplification (pending state removal)

[gzanitti]

This PR simplifies the operation of the assembler, mainly by removing the need to maintain a pending state and to return the assembler to its original state in certain situations (which caused issue #124).

The main changes include

• The creation of two structs, PendingMacro and PendingLabel, which store information about the macros and labels used before they were defined, as well as their relative position in the code.
• Some push operations have been changed to take an option as parameter, and if this is not None, to act more like an insert than a push (useful for example to insert the calls to the pending macros).

Also, issue #124 has been fixed and the corresponding test added.

All tests should pass successfully. However, I would like to test the new assembler a bit more by writing some code (e.g. following @lightclient 's recommendation and writing an ERC-20 implementation using ETK) before merging it.

In case you don't find it necessary, it can be merged right now.

[gzanitti]

@lightclient @SamWilsn the assembler rewrite is ready. If anyone has time during the week to review the code, I'll be happy to address your comments :)

P.S: I'm tagging you two because you're the two people I've been talking to about the assembler rewrite. If you don't have time, feel free to ignore this message.

[SamWilsn]

@gzanitti we're the correct people to tag for sure! I'm swamped for the rest of the week, but I start my vacation next week so I should finally have some time for ETK 🎉

[gzanitti]

@gzanitti we're the correct people to tag for sure! I'm swamped for the rest of the week, but I start my vacation next week so I should finally have some time for ETK 🎉

That's great! :). Anyway, today I was playing around with an ERC20 implementation I was working on (basically moving various parts of the code into macros, adding includes/imports) and I found a bug. Which I thought I had fixed a few hours ago, but it turns out that the new change breaks some "push label"s haha.

I'll try to fix it in the next few days, but I'm starting to think it might need a bit of a major rewrite. I'll keep you posted.

[SamWilsn]

That might be the best approach. Keep the pest parser and scrap the rest.

…

On Wed, Sept 27, 2023, 18:39 Gastón Zanitti ***@***.***> wrote: @gzanitti <https://github.com/gzanitti> we're the correct people to tag for sure! I'm swamped for the rest of the week, but I start my vacation next week so I should finally have some time for ETK 🎉 That's great! :). Anyway, today I was playing around with an ERC20 implementation I was working on (basically, moving different parts of the code to macros, adding includes/imports) and I found a bug. Which I thought I had fixed a few hours ago, but it turns out that the new change breaks some "push label"s haha. I'll try to solve it in the next few days, but I'm starting to think it might need a bit of a major rewrite. I'll keep you posted. — Reply to this email directly, view it on GitHub <#133 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANU4EQOXIUOWPKZQGSYCE5DX4STJXANCNFSM6AAAAAA46VVE2I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[gzanitti]

That might be the best approach. Keep the pest parser and scrap the rest.

Excellent. I tried not to do that from the very beginning, because I was trying to preserve as much of the original code as possible (I didn't want to be the outsider who comes to break everything). You're going to say it wasn't necessary, but it's good to have your approval to make a deeper change haha.

I have some ideas I'd like to try that I think will help simplify the whole compiler.

I'll keep you posted. Thanks!

[gzanitti]

Small update:

I'm almost done with the new code. The truth is: There are no big changes, just some reorganizations.

Roughly, these are the major changes:

• After the code is parsed, I take care of resolving each include and import, so that the code is as "expanded" as possible.

  • Biggest gain: This allows me to create a new assembler to handle the includes, and makes handling scopes easier.
  • Biggest loss: After it comes out of the parser, I have to preprocess the code. I tried to do it all in one pass, but the code got too messy and I preferred to prioritize readability (at least for now, we can optimize it in the future).

• Thanks to the previous point, I also take the opportunity to save all macros before I start assembling. Having all the macros predefined saves a lot of headaches, since calling a macro before declaring it means expanding part of the code in the future. Again, maybe it's not as efficient as it could be, but I prefer clarity over performance for now. Everything can be optimized in a future PR.

• Having all the macros already defined also helped me to simplify the assembler I had already defined to remove the pending state at the beginning of this PR, so there is no need to do inserts anymore, all new operations are placed at the end of the list, making the code much simpler and more understandable.

I still need to fix a few things. Most importantly, I would like to add some more tests that make use of the dynamic push of labels (%push(label)) to make sure everything is working properly. Also, the handling and resolution of paths in each include/import is just a "hacky patch". And there is a test that fails when the push label is an expression. Nothing that is likely to be a major headache, but I'd like to have it sorted out before you guys take a look at the code.

Anyway, I'm really enjoying working on this project, so I hope you like my ideas.

P.S.: I'm not a Rust expert, my code might not be "rusty" enough, so I'm open to any suggestions :).

P.S.2: I'm really starting to get attached to this project. Would you mind if I try to put together a logo and a small website? Of course I plan to open a thread where we can discuss ideas/designs. I'd really like to help spread the word about ETK. Hi Huff, I'm looking at you ;)

[gzanitti]

I think the code is ready 🚀
However, I'll be adding a few more tests over the next days to be sure everything works perfectly.

Thanks!

Edit: II was confused about the tests that were supposedly wrong. The corrections have already been made.

[SamWilsn]

First few comments. Looking reasonable so far! I'm about halfway through asm.rs.

[gzanitti]

First few comments. Looking reasonable so far! I'm about halfway through asm.rs.

Thanks! Take your time and don't hesitate to ask questions or correct anything you think can be improved.
Especially if something can be written in a more "rusty" way.

[gzanitti]

In addition to issue 124, mentioned at the beginning, this PR also fixes issue #108 (both tests mentioned in the issue were added to the test suite) and issue #82

[SamWilsn]

Another partial review. Sorry!

[gzanitti]

Another partial review. Sorry!

Thank you for your comments @SamWilsn. I just hope I'm not giving you too many headaches haha.

P.S: You can take your time on the review, in the meantime I'm finishing PR #135 and, if necessary, I can spend some time on solving some other items on the list of issues in the meantime.

[SamWilsn]

P.S.2: I'm really starting to get attached to this project. Would you mind if I try to put together a logo and a small website? Of course I plan to open a thread where we can discuss ideas/designs. I'd really like to help spread the word about ETK. Hi Huff, I'm looking at you ;)

I missed this part! I'd absolutely love that! There are so many little things I've wanted to build with ETK, and I had basically given up on having the time/motivation to do it. Extremely happy to have someone as excited as you. A website (maybe with an ETK-compiled-to-JavaScript playground 😇) is a great idea!

[gzanitti]

P.S.2: I'm really starting to get attached to this project. Would you mind if I try to put together a logo and a small website? Of course I plan to open a thread where we can discuss ideas/designs. I'd really like to help spread the word about ETK. Hi Huff, I'm looking at you ;)

I missed this part! I'd absolutely love that! There are so many little things I've wanted to build with ETK, and I had basically given up on having the time/motivation to do it. Extremely happy to have someone as excited as you. A website (maybe with an ETK-compiled-to-JavaScript playground 😇) is a great idea!

Great, glad to be able to help, and to discuss and develop all the ideas you have in mind. I'll prepare a prototype in the next few weeks, including the playground you mention, and I'll write to you to see what you think 😄

[SamWilsn]

Sorry it's taken me so long to re-review! I believe I found another case that passed before this pull request and is failing now.

Thanks again for being so persistent on this. I really appreciate the help getting ETK into a good place <3

[gzanitti]

Sorry it's taken me so long to re-review! I believe I found another case that passed before this pull request and is failing now.

Thanks again for being so persistent on this. I really appreciate the help getting ETK into a good place <3

Sorry, it's also my fault for having to change several parts of the code in the middle of the process and make you check everything again. As I always say, thanks for your patience and for letting me collaborate :)

Regarding the code, I solved the case you mention and also added some more tests. The novelty is that now all the pushes are checked and updated taking into account the previous updates (acum variable) (declared_labels is now an ordered structure because it matters the order when adding the accumulated size to the labels).

[gzanitti]

Hey @SamWilsn, I needed a HashMap sorted by insert and ended up adding IndexMap to the project. However, it is only used in the assembler and could be replaced by a struct with a HashMap and a Vector if you don't want to add more dependencies.

Let me know what you think.

[SamWilsn]

Uggh, I found another test case that fails with this approach. I'll push it up in a second. I'm so sorry x.x

[SamWilsn]

I'm totally happy with the IndexMap.

[SamWilsn]

Pushed the new test. Summary is:

	Variable + Fixed	Variable + Variable
Source	%push(lbl1 - lbl2) push2 lbl1 + lbl2 pc # repeat 126 times. lbl1: lbl2:	%push(lbl1 - lbl2) %push(lbl1 + lbl2) pc # repeat 126 times. lbl1: lbl2:
Outcome	6000610106...	6000610108...

The old code compiled these to the same output, where the new code has different outputs.

[gzanitti]

Hey @SamWilsn , how are you?
First of all, sorry for the delay. I've been having some personal issues and couldn't dedicate as much time as I would have liked to ETK. Also, my country is slowly going into the abyss, which doesn't help.

Don't worry about mentioning as many corrections as you deem necessary. After all, this code belongs more to you than to me and I can make all the changes you consider necessary until you are satisfied :).

Regarding the bug you reported, I'm sorry I didn't see it before, I understand that with the last change nothing like this should happen again.

I look forward to your comments!

[SamWilsn]

Just these last two items!

[gzanitti]

Just these last two items!

Excellent. Thank you @SamWilsn for your time and patience. Both changes have been made :)