-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Possible addition gbz80-pseudoOps #135
Comments
First, let's get one thing straight. I see the intention and effort that went into this. I believe it was also the case to the currently three persons who left you a thumbs up. [EDIT: forgot to mention this] Some of your work, especially the addresses, would be good to integrate into hardware.inc. The first one is that it's really unoptimized - this on its own is, imo, enough to reject it on the awesome list. Instead, I've sent a couple PRs to hopefully address this. The second problem is basically that I'm firmly opposed to pseudo-op macros. This can be broken down into three sub-problems: a sharp loss in readability, being a major hindrance to optimization, and just being a huge footgun. Below is a more detailed explanation of each of those. Readability It's fine, since the macros are simple... but then, why have the macros at all? Is this worth requiring additional knowledge from maintainers?
Why not just write the two lines? They're small, they're not confusing. Further, let's assume you want to OR two values from memory. Alright:
But you find you also need to
is a good option, but it's not consistent...
is very consistent, but it generates a Optimization
which is known to be optimizable to the following (1 byte, 1 cycle):
My point is, the macros are a major barrier to optimization, because they group some instructions that could otherwise be analyzed as a whole, split, and optimized. Another important thing is that they also constrain the way the code is written: Further, to prevent side effects, your
Footgun potential
It's my first time seeing it, but I read the comment and the name (but not the code, because comments should be enough). I deduce that it's performing a OR, which I know is a commutative operation, so I can do this!
But it doesn't compile! Why? The other huge problem is side effects. There are two possibilities: either you have as little side effects as possible by spamming To sum all this up, I think pseudo-op macros should not be used period, and thus I'm against including them in the awesome list. I'm open for debate, though. |
I originally started writing a comment, but I waited because I knew @ISSOtm would write a comment like that one 😉 So I left a 👍 to show my support in the meantime. His analysis of the library itself is good, and more insightful than what I could make. These are all issues that can be fixed, however (mostly by documenting - side effects of a pseudo-op, for example, aren't any worse than side effects of regular ops, but both obviously need to be documented), and these issues are not mentioned in the summary so I think we can disregard them for the broader discussion:
This is a more interesting debate. I don't think personally being against pseudo-ops (which I also am; I'm also against CamelCase!) is grounds for not including them on the list. I also wouldn't use C to program a Game Boy game, but we still have lots of C resources on there. Like C, these pseudo-ops have one important goal: Make programming Game Boy games more accessible. But these pseudo-ops are even better than C: They can make programming Game Boy games in assembly more accessible! I dislike boilerplate in code. Anything that can remove boilerplate is good, in my book. There's a reason most of us use the same Now, the magical routines that we replace the boilerplate code with should of course work, and they should be documented well. (People should also know what the actual ops of the GB CPU are before resorting to pseudo-ops, but luckily our list covers that already.) If those two things are true, I see nothing wrong with including them on the list. |
I'm making a difference there; I'm against pseudo-ops period, whereas I'm against C in some cases (roughly anything but programming under time constraints, = game jams). That's why I'm OK with C on the awesome list, but not with this.
I'm not sure it makes it more accessible... due to the footgun problem, which newbies will probably not be aware of. In fact, I'd consider pseudo-ops to be harmful and should be avoided for everyone but experienced developers.
I'm not sure what you call "boilerplate" here. |
This is cool! I'm late to the party but I'd very much like to see this in the list. @ISSOtm observations are on point and he raised some clear issues. @joeldipops do you think you can add some documentation to address this points? I've seen @ISSOtm opened a couple of PR on your repository. Can you take a look at them? Keep us updated! |
Yep, I do see ISSOtm's points for a lot of things, and addressing them has been at the back of my mind for a while. Needless to say I disagree that pseudo-ops are 'always' an anti-pattern, but I definitely see the issue with some of these, in particular, ops with effects like and/or/xor. I don't agree that "at this point, you're not writing ASM anymore, you are making your own language." is a problem. My goal was a clean interface and implementation details can be black-boxed. If I can improve the library's usability, maintain its performance, and reduce its foot-shoot-potential by stuffing it with lots of rgbasm macro-language, I'd like to pursue that. I've started down that path, though I need to check my docs are up to date with it. As for the specific pull requests that were raised, yep, I've looked at those and incorporated some changes, though there's room for improvement there. At present I'm working on the N64 side of my main project, so I won't be looking in depth at this until I swing back towards the Game Boy, but if there's any more suggestions/pull requests/bugs in the mean time I'll definitely take a look. |
I've reviewed the project and opened issues and PRs on problems. |
I didn't "just close" anything. I closed pull requests after making some efforts to incorporate their suggestions into my own commits, and opened new issues relating to them where I understand further work needs to be done. |
Do we have any update on this, @joeldipops ? This also looks interesting: https://github.com/joeldipops/TransferBoy |
I have made quite a few improvements to it, especially based on your and ISSOtm's advice. There's plenty of room for improvement yet, especially on the documentation side of things. That said I haven't touched it in a while since I've been focusing on TransferBoy and a new baby. Thanks for taking a look. |
Great! I really appreciate you sticking with it and working on feedbacks.
That's so cool! Congratulations! 😍 I think this basically reduces to a general stance we have to take on Macros/these type of black-boxing |
I have been working on a library that I think other Game Boy developers could make use of. It is hosted here https://github.com/joeldipops/gbz80-pseudoOps
I'm spruiking my own work so I don't feel qualified to answer the question "Is it awesome" but I think it fits the three criteria
It must be in a minimal working state;
Although I intend for the library to continue to grow and improve, I believe in its current state it is already useful. The 20 or so macros already written are working and reasonably well documented.
It must have a clear purpose or provide something interesting
The purpose is to
a) polyfill operations that feel like they could exist on the gameboy CPU but don't, for example moving data from one memory address to another
ldAny [LcdControl], LCD_ON
or 16 bit subtractionsub16 HL, BC
b) Make rgbasm code more readable, through the above polyfills and intuitive constant names.
It must provide a minimal documentation briefly describing what is the project and how to make use of it.
There's a README.md with instructions for including the library and documenting most of the macros, including size, cycle counts and flags affected.
My only concern is that I'm worried I'm the only person that would find this stuff useful. I haven't seen anyone else that dislikes gbhw.inc and it's "rSCY" style naming, and I've completely ignored some advice I've read not to indent asm code as I would an OOP language.
The text was updated successfully, but these errors were encountered: