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

Cleanup of code: Removing uncessecary `fpo' function argument, Replacing character numbers with their corresponding escape codes #2

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

oliverkwebb
Copy link

This is some minor cleanup of the code with no big alterations to the logic or function of the program. Making fpo a global variable and removing it as a function argument for the *_or_die() functions and xxdline() because those functions are never invoked on anything but fpo. And replacing most of the numbers that represent ASCII characters with their corresponding escape codes, since '\r' is easier to interpret than 13.

Oliver Webb added 2 commits January 30, 2024 22:56
most internal functions. Make global variable and remove function
arguments.
@xyproto
Copy link
Owner

xyproto commented Feb 1, 2024

Thanks for the PR!

I like most of the changes, but isn't introducing a global variable a step in the wrong direction? At least to me, ideally, there should be as little global state as possible, and functions should be as pure and "functional" as possible.

@oliverkwebb
Copy link
Author

oliverkwebb commented Feb 1, 2024

While yes, I would mostly agree that global variables are bad and there should be as little global state as possible. I felt like the code would be cleaner without having to specify fpo in the output functions.

Error messages use fprintf(stderr, ...) and xxd never acts like tee, it always has one output. So for most of the output functions it wouldn't make sense to specify anything but fpo.

And the only reason fpo really exists in the code is because xxd can output to files, so fpo means "stdout, unless there is a second file argument, then that file". It's never modified beyond argument parsing.

If the original xxd didn't have a built-in "output to file" feature. The code would have used the global variable stdout.

I WOULD suggest modifying stdout since it's not const and you can do stdout = fopen(... making fpo completely redundant. But that breaks on different implementations of libc and is generally hack-y since the fileno of stdout and STDOUT_FILENO can be different

Oliver Webb added 2 commits January 31, 2024 20:42
This reverts commit 7746c23.

Breaks on musl because stdout is const on musl systems
@xyproto xyproto merged commit 1a2edc4 into xyproto:main Feb 1, 2024
1 check passed
@xyproto
Copy link
Owner

xyproto commented Feb 1, 2024

I agree with the reasoning! Thanks for testing it on musl as well. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants