-
Notifications
You must be signed in to change notification settings - Fork 40
Murcake/x86 dataclasses #4
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.
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
Murcake/x86 dataclasses #4
Conversation
The argument to The argument to The two arguments to The argument to |
If Global's argument is a string, why does it explicitly do |
d1eb262
to
6cd4a5e
Compare
I don't know why |
True. Just was wondering if it was ever intended be used on more than just strings |
Anyway, everything is ready then. I also have another PR that is fixing a bug a bit complicating the homework. |
6cd4a5e
to
d52fdf1
Compare
d52fdf1
to
620f0e3
Compare
Did you possibly get enough time, @Fyrbll? |
Thanks for the reminder. Revisiting this now. |
Although it is true that in this course we always make the argument of EDIT. Formatting of quote. |
that improved typing information and removes a lot of boilerplate The semantics should remain completely the same, with the exception, that the classes are now immutable. It was expected anyway, given that they had a `__hash__` method.
620f0e3
to
e52d3d0
Compare
Done |
Thanks for seeing those updates through. I want to suggest these additional changes that ensure X86Program Don't set X86ProgramDefs Don't set Instr Change the definition of the class
Anything that is a Subclass of Instr To ensure we don't break anything we want to somehow retain For example to pass the existing tests we can subsequently change
I also mentioned "unless otherwise specified ... previous version of
|
Just for reference here's a minimal sequence of changes that will allow the reference compiler to pass all tests.
To provide a better answer I will need to examine why the "default" dataclass-derived hash function for |
I'm going to do that, but just to see if feasible, it seems that the more common approach here is storing these additional properties in a hash-table, rather than monkey-patching? Would that work, or would it require more effort to further adapt?
That is not the best definition of
It's indeed interesting why it's required. Could it be because of the
I'm not sure I completely get it. Do tests rely on a particular implementation of |
@Fyrbll could you please check if the latest commit fixes the issues you mentioned? |
edd0c83
to
c4595ea
Compare
ping |
This commit fixes the issues I mentioned and passes all tests. It should be fine to merge.
Since @murcake do I have the right idea about the |
I don't frankly remember why it was done. My guess is that the classes uses mutable dicts and lists, so I thought that it was designed to be mutable, so it shouldn't be frozen. If you think this guess is valid, let's leave it that way, if not—I'll reintroduce frozenness.
The main reasoning I believe, as above, was that
Yes! |
Yes, I meant that they should not be frozen and I wanted to record that you made this change. Apologies for the confusion.
I get it now. Thanks for the comment about immutability. I was only thinking in terms of |
I haven't turned some classes to dataclasses, mainly because I don't know what should be the types of their arguments.
It would be very helpful if someone pointed them out.
The classes in question are:
X86ProgramDefs
,IndirectJump
,TailJump
,Global