xyproto / tinyxxd

Drop-in replacement and standalone version of the hex dump utility that comes with ViM
Other
21 stars 4 forks source link

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

Closed oliverkwebb closed 8 months ago

oliverkwebb commented 8 months ago

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.

xyproto commented 8 months ago

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 commented 8 months ago

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

xyproto commented 8 months ago

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