Skip to content
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

Remove global variables from GNU z80 disassembler #4343

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

moste00
Copy link
Contributor

@moste00 moste00 commented Mar 7, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

This change removes global variables from librz\arch\isa_gnu\z80. This is a fairly massive change, so sorry in advance for any reviewer, most of the diff is trivial.

The global variables that were removed were shared state that multiple functions in the file could modify, this is in essence the "OO" pattern, the functions that modified the variables were "methods" that operated on the same implicit "object". This pattern were made explicit by moving the variables into a structure, and all the functions were changed so that they accept a pointer to the structure as the first argument. The function that instantiates the structure is the one that starts the assembly process, assemble. This function outlives all other functions, ensuring the pointer it passes to the others is valid.

In addition to that change, several minor cleanups were also made:

  • Dead code, presumably added for debugging, was removed (see: the variable cont)

  • Dead code branching on the value of the variable define_macro was removed, since the variable has one value that no other code modifies.

  • Dead code that was nested under an #if 0 preprocessor directive. The function that used to contain this code is now empty, to be removed in a future cleanup (it's an error logging function that was presumably useful in the original context of the code)

More cleanups to this file is later needed, to remove the function printerr and to free the memory returned by strdup in the initialization of z80buffer.

Test plan

No new functionality was added so no new tests are necessary. The branch builds and all the tests in db/asm/z80 pass.

Closing issues

...

Comment on lines 46 to 47
static void printerr(int error, const char *fmt, ...) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the original code it was also empty but there was code in its body guarded under an #if 0 preprocessor directive. I saw there was no good reason to keep maintaining that code since it's essentially comments, and I deleted it.

This function in some sense "deserves" deletion anyway because it's not following the same general approach to error reporting followed in the rest of the project, it prints directly to stderr and uses a weird va_list type to manipulate variadic arguments. I leave it empty instead of completely removing it because other functions in expression.c use it a lot, I plan to replace it with multiple functions or by writing to an error buffer in the state structure in a future PR. I didn't do any of this in this PR because it's already massive as it is.

Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome job.

@wargio wargio added the enhancement New feature or request label Mar 8, 2024
@wargio wargio changed the title Remove global variables, replacing it with a state structure Remove global variables from GNU z80 disassembler Mar 8, 2024
@wargio
Copy link
Member

wargio commented Mar 10, 2024

something is wrong when you compile it.

@moste00
Copy link
Contributor Author

moste00 commented Mar 10, 2024

something is wrong when you compile it.

Yeah very sorry for that. I previously blamed it on the original code but it turns out that I did remove a few const qualifiers from the function signatures here and there and all those warnings are because of that. I put the 3 qualifiers I removed back and hopefully those are all the ones I deleted.

I moved the function printerrr into the .h file in an attempt to make the diff of expressions.c cleaner and smaller, alas git still thinks I removed the entire file and wrote it again. I'm open to any suggestions that make git see that I didn't change anything except the function signature and a few lines in the body of each function.

@wargio
Copy link
Member

wargio commented Mar 11, 2024

if printerrr is an empty function, i strongly suggest to remove it and its usages.

@moste00 moste00 force-pushed the z80_asm_rm_global_vars branch 2 times, most recently from 6a4d738 to 813ca1d Compare March 12, 2024 00:12
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much better.

@wargio wargio merged commit 672d0f7 into rizinorg:dev Mar 12, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants