Refactor shell_batch.cpp #2342
Refactor shell_batch.cpp #2342
Conversation
FeralChild64
added
the
cleanup
There was a problem hiding this comment.
Very nice cleanup, @MeAreJeenius !
Mostly minor comments.
My main concern is that we don't have regressions; batch processing is so variable and nuanced, I'm not sure if there are some test batch files out there that we can feed through this to make sure all the corner cases are hit (and that we're not dropping functionality).
|
@MeAreJeenius , are you on Linux or macOS? If so, can you run it through UBASAN: meson setup -Dbuildtype=debug -Db_sanitize=address,undefined build/ubasan
meson compile -C build/ubasanBinary: Also, set this to catch all ASAN issues: export ASAN_OPTIONS=strict_string_checks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:detect_leaks=1With those in place, run the binary against a test suite of batch files. If there are any issues, the sanitizer instrumentation will report it in the console output. |
MeAreJeenius
force-pushed
the
shell-batch-refactor
branch
from
b047889 to
f7157ea
Compare
|
@kcgen Good call on the sanitizers. No clue why I didn't use them sooner in these commits. Turns out, strncpy doesn't guarantee a '\0' at the end like snprintf, so strlen would trigger a stack buffer overflow (fixed now). Also found an initialization-order-fiasco right at startup. |
MeAreJeenius
force-pushed
the
shell-batch-refactor
branch
from
f7157ea to
8cb23d6
Compare
Replace char buffers with std::string. Split up into a few functions. Simplify the control flow. Remove the cycle hack, as it doesn't appear to do much. Cycles can be set within batch files without it. Properly identify labels and comments when preceeded by tabs, equals signs, and spaces. Truncate commands that are too long instead of crashing. Later commits should refactor the function and the code that calls it so that it uses a std::string instead of a fixed size char buffer.
Break into several functions. Simplify control flow. Allow colon to be preceeded by spaces tabs and equals signs.
Run clang-format on the file. Remove unnecessary initializers from the constructor. Remove undeeded headers. Rename char constants, and remove uncessary ones. If file is not openable, give warning instead of error.
MeAreJeenius
force-pushed
the
shell-batch-refactor
branch
from
8cb23d6 to
88d173f
Compare
|
In addition to implementing the feedback, I reworked
|
|
Very nice refactoring, @MeAreJeenius! I think we're clear to merge 🚀 (PS- MSYS2 on GitHub's side has broke FluidSynth's unit tests; this isn't due to any change on the repo's side, as it was previously running in the last main build here: https://github.com/dosbox-staging/dosbox-staging/actions/runs/4440895580) |
No description provided.