From 7c6f778ae7a38a96009d3952be4d581f3cc738b6 Mon Sep 17 00:00:00 2001 From: Rangi <35663410+Rangi42@users.noreply.github.com> Date: Fri, 2 May 2025 16:44:12 -0400 Subject: [PATCH] Take care of miscellaneous commented TODOs (#1676) --- CMakeLists.txt | 4 +-- contrib/gbdiff.bash | 5 ++-- src/fix/main.cpp | 8 +++--- src/gfx/main.cpp | 6 ++--- src/gfx/process.cpp | 10 +------- src/gfx/reverse.cpp | 2 -- test/asm/label-diff.asm | 8 ++---- test/asm/label-diff.err | 54 ++++++++++++++++++++-------------------- test/asm/unique-id.asm | 8 +++--- test/asm/unique-id.err | 14 ++++++++++- test/gfx/rgbgfx_test.cpp | 9 +------ 11 files changed, 60 insertions(+), 68 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c88d1faf..c62a0e9d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -51,8 +51,8 @@ else() add_link_options(${SAN_FLAGS}) add_definitions(-D_GLIBCXX_ASSERTIONS) # A non-zero optimization level is desired in debug mode, but allow overriding it nonetheless - # TODO: this overrides anything previously set... that's a bit sloppy! - set(CMAKE_CXX_FLAGS_DEBUG "-g -Og -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE STRING "" FORCE) + set(CMAKE_CXX_FLAGS_DEBUG "-g -Og -fno-omit-frame-pointer -fno-optimize-sibling-calls ${CMAKE_CXX_FLAGS_DEBUG}" + CACHE STRING "" FORCE) endif() if(MORE_WARNINGS) diff --git a/contrib/gbdiff.bash b/contrib/gbdiff.bash index 9d852937..a1c6fa33 100755 --- a/contrib/gbdiff.bash +++ b/contrib/gbdiff.bash @@ -32,14 +32,15 @@ diff <(xxd "$1") <(xxd "$2") | while read -r LINE; do # Ignore comment lines, only pick matching bank # (The bank regex ignores comments already, make `cut` and `tr` process less lines) grep -Ei "$(printf "^%02x:" $BANK)" "$SYMFILE" | + sed "s/$(printf "^%02x:" $BANK)/0x/g" | cut -d ';' -f 1 | tr -d "\r" | + sort -g | while read -r SYMADDR SYM; do - SYMADDR=$((0x${SYMADDR#*:})) + SYMADDR=$(($SYMADDR)) if [[ $SYMADDR -le $ADDR ]]; then printf " (%s+0x%x)\n" "$SYM" $((ADDR - SYMADDR)) fi - # TODO: assumes sorted sym files done | tail -n 1 fi) printf "%02x:%04x %s\n" $BANK $ADDR "$EXTRA" diff --git a/src/fix/main.cpp b/src/fix/main.cpp index 8e636543..b75382f7 100644 --- a/src/fix/main.cpp +++ b/src/fix/main.cpp @@ -719,8 +719,7 @@ static bool hasRAM(MbcType type) { case MBC3_TIMER_BATTERY: case MBC5: case MBC5_RUMBLE: - case MBC6: // TODO: not sure - case BANDAI_TAMA5: // TODO: not sure + case BANDAI_TAMA5: // "Game de Hakken!! Tamagotchi - Osutchi to Mesutchi" has RAM size 0 case MBC_NONE: case MBC_BAD: case MBC_WRONG_FEATURES: @@ -741,6 +740,7 @@ static bool hasRAM(MbcType type) { case MBC5_RAM_BATTERY: case MBC5_RUMBLE_RAM: case MBC5_RUMBLE_RAM_BATTERY: + case MBC6: // "Net de Get - Minigame @ 100" has RAM size 3 (32 KiB) case MBC7_SENSOR_RUMBLE_RAM_BATTERY: case POCKET_CAMERA: case HUC3: @@ -1276,7 +1276,7 @@ static bool processFilename(char const *name, char const *outputName) { // LCOV_EXCL_START report("FATAL: Failed to stat \"%s\": %s\n", name, strerror(errno)); // LCOV_EXCL_STOP - } else if (!S_ISREG(stat.st_mode)) { // TODO: Do we want to support FIFOs or symlinks? + } else if (!S_ISREG(stat.st_mode)) { // We do not support FIFOs or symlinks // LCOV_EXCL_START report( "FATAL: \"%s\" is not a regular file, and thus cannot be modified in-place\n", @@ -1531,7 +1531,7 @@ int main(int argc, char *argv[]) { "warning: RAM size 1 (2 KiB) was specified for MBC \"%s\"\n", mbcName(cartridgeType) ); - } // TODO: check possible values? + } } else if (ramSize) { fprintf( stderr, diff --git a/src/gfx/main.cpp b/src/gfx/main.cpp index 06a1187b..4ad0dff9 100644 --- a/src/gfx/main.cpp +++ b/src/gfx/main.cpp @@ -451,9 +451,9 @@ static char *parseArgv(int argc, char *argv[]) { } else { options.palSpecType = Options::EXPLICIT; // Can't parse the file yet, as "flat" color collections need to know the palette - // size to be split; thus, we defer that - // TODO: this does not validate the `fmt` part of any external spec but the last - // one, but I guess that's okay + // size to be split; thus, we defer that. + // If a following `-c` overrides a previous one, the `fmt` part of an overridden + // external palette spec will not be validated, but I guess that's okay. localOptions.externalPalSpec = musl_optarg; } break; diff --git a/src/gfx/process.cpp b/src/gfx/process.cpp index 8506ab9f..698bbf5e 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -211,11 +211,6 @@ public: png_set_read_fn(png, this, readData); png_set_sig_bytes(png, pngHeader.size()); - // TODO: png_set_crc_action(png, PNG_CRC_ERROR_QUIT, PNG_CRC_WARN_DISCARD); - - // Skipping chunks we don't use should improve performance - // TODO: png_set_keep_unknown_chunks(png, ...); - // Process all chunks up to but not including the image data png_read_info(png, info); @@ -294,9 +289,7 @@ public: options.verbosePrint(Options::VERB_INTERM, "No embedded palette\n"); } - // Set up transformations; to turn everything into RGBA888 - // TODO: it's not necessary to uniformize the pixel data (in theory), and not doing - // so *might* improve performance, and should reduce memory usage. + // Set up transformations to turn everything into RGBA888 for simplicity of handling // Convert grayscale to RGB switch (colorType & ~PNG_COLOR_MASK_ALPHA) { @@ -559,7 +552,6 @@ static void generatePalSpec(Png const &png) { static std::tuple, std::vector> generatePalettes(std::vector const &protoPalettes, Png const &png) { // Run a "pagination" problem solver - // TODO: allow picking one of several solvers? auto [mappings, nbPalettes] = overloadAndRemove(protoPalettes); assume(mappings.size() == protoPalettes.size()); diff --git a/src/gfx/reverse.cpp b/src/gfx/reverse.cpp index 3542dee6..0bdc0c53 100644 --- a/src/gfx/reverse.cpp +++ b/src/gfx/reverse.cpp @@ -195,8 +195,6 @@ void reverse() { Options::VERB_INTERM, "Reversed image dimensions: %zux%zu tiles\n", width, height ); - // TODO: `-U` to configure tile size beyond 8x8px ("deduplication units") - std::vector, 4>> palettes{ {Rgba(0xFFFFFFFF), Rgba(0xAAAAAAFF), Rgba(0x555555FF), Rgba(0x000000FF)} }; diff --git a/test/asm/label-diff.asm b/test/asm/label-diff.asm index f9d3e7df..9a321015 100644 --- a/test/asm/label-diff.asm +++ b/test/asm/label-diff.asm @@ -1,6 +1,3 @@ - -PUSHS - SECTION "floating", ROM0 Known: ; This symbol is known to be a label, but its value is not known yet @@ -15,15 +12,14 @@ SECTION "fixed 2", ROM0[69] Constant2: ; Same as above +ENDSECTION ; Ensure we are in neither section + MACRO print_diff PRINTLN (\1) - (\2) PRINTLN (\2) - (\1) ENDM -POPS ; Ensure we are in neither section - -; TODO: uncomment all that can be, there is seriously room for improvement here ; Diffing two constants should work print_diff Constant, Constant2 ; Diffing two labels in the same SECTION as well diff --git a/test/asm/label-diff.err b/test/asm/label-diff.err index 279b040b..916b6d22 100644 --- a/test/asm/label-diff.err +++ b/test/asm/label-diff.err @@ -1,37 +1,37 @@ +error: label-diff.asm(28) -> label-diff.asm::print_diff(19): + Expected constant expression: 'Known' is not constant at assembly time +error: label-diff.asm(28) -> label-diff.asm::print_diff(20): + Expected constant expression: 'Known' is not constant at assembly time +error: label-diff.asm(30) -> label-diff.asm::print_diff(19): + Expected constant expression: 'Unknown' is not constant at assembly time +error: label-diff.asm(30) -> label-diff.asm::print_diff(20): + Expected constant expression: 'Unknown' is not constant at assembly time +error: label-diff.asm(32) -> label-diff.asm::print_diff(19): + Expected constant expression: 'Known' is not constant at assembly time error: label-diff.asm(32) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Known' is not constant at assembly time -error: label-diff.asm(32) -> label-diff.asm::print_diff(21): - Expected constant expression: 'Known' is not constant at assembly time + Expected constant expression: 'Unknown' is not constant at assembly time +error: label-diff.asm(34) -> label-diff.asm::print_diff(19): + Expected constant expression: 'Unknown' is not constant at assembly time error: label-diff.asm(34) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(34) -> label-diff.asm::print_diff(21): - Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(36) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Known' is not constant at assembly time -error: label-diff.asm(36) -> label-diff.asm::print_diff(21): - Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(38) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(38) -> label-diff.asm::print_diff(21): Expected constant expression: 'Unknown2' is not constant at assembly time -error: label-diff.asm(45) -> label-diff.asm::print_diff(20): +error: label-diff.asm(41) -> label-diff.asm::print_diff(19): Expected constant expression: 'Known' is not constant at assembly time -error: label-diff.asm(45) -> label-diff.asm::print_diff(21): +error: label-diff.asm(41) -> label-diff.asm::print_diff(20): Expected constant expression: 'Known' is not constant at assembly time -error: label-diff.asm(47) -> label-diff.asm::print_diff(20): +error: label-diff.asm(43) -> label-diff.asm::print_diff(19): Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(47) -> label-diff.asm::print_diff(21): +error: label-diff.asm(43) -> label-diff.asm::print_diff(20): + Expected constant expression: 'Unknown' is not constant at assembly time +error: label-diff.asm(54) -> label-diff.asm::print_diff(19): + Expected constant expression: PC is not constant at assembly time +error: label-diff.asm(54) -> label-diff.asm::print_diff(20): + Expected constant expression: PC is not constant at assembly time +error: label-diff.asm(56) -> label-diff.asm::print_diff(19): + Expected constant expression: 'Known' is not constant at assembly time +error: label-diff.asm(56) -> label-diff.asm::print_diff(20): + Expected constant expression: PC is not constant at assembly time +error: label-diff.asm(58) -> label-diff.asm::print_diff(19): Expected constant expression: 'Unknown' is not constant at assembly time error: label-diff.asm(58) -> label-diff.asm::print_diff(20): Expected constant expression: PC is not constant at assembly time -error: label-diff.asm(58) -> label-diff.asm::print_diff(21): - Expected constant expression: PC is not constant at assembly time -error: label-diff.asm(60) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Known' is not constant at assembly time -error: label-diff.asm(60) -> label-diff.asm::print_diff(21): - Expected constant expression: PC is not constant at assembly time -error: label-diff.asm(62) -> label-diff.asm::print_diff(20): - Expected constant expression: 'Unknown' is not constant at assembly time -error: label-diff.asm(62) -> label-diff.asm::print_diff(21): - Expected constant expression: PC is not constant at assembly time error: Assembly aborted (18 errors)! diff --git a/test/asm/unique-id.asm b/test/asm/unique-id.asm index a689686b..6b69eecb 100644 --- a/test/asm/unique-id.asm +++ b/test/asm/unique-id.asm @@ -7,9 +7,9 @@ macro m ENDR warn_unique endm - ; TODO: Ideally we'd test now as well, but it'd cause a fatal error - ;warn_unique - m - ;warn_unique + + warn_unique + m + warn_unique m warn_unique diff --git a/test/asm/unique-id.err b/test/asm/unique-id.err index 1cd180b7..11392fa6 100644 --- a/test/asm/unique-id.err +++ b/test/asm/unique-id.err @@ -1,3 +1,9 @@ +error: unique-id.asm(11): + '\@' cannot be used outside of a macro or REPT/FOR block +while expanding symbol "warn_unique" +warning: unique-id.asm(11): [-Wuser] + ! +while expanding symbol "warn_unique" warning: unique-id.asm(12) -> unique-id.asm::m(4): [-Wuser] _u1! while expanding symbol "warn_unique" @@ -10,6 +16,12 @@ while expanding symbol "warn_unique" warning: unique-id.asm(12) -> unique-id.asm::m(8): [-Wuser] _u1! while expanding symbol "warn_unique" +error: unique-id.asm(13): + '\@' cannot be used outside of a macro or REPT/FOR block +while expanding symbol "warn_unique" +warning: unique-id.asm(13): [-Wuser] + ! +while expanding symbol "warn_unique" warning: unique-id.asm(14) -> unique-id.asm::m(4): [-Wuser] _u4! while expanding symbol "warn_unique" @@ -28,4 +40,4 @@ while expanding symbol "warn_unique" warning: unique-id.asm(15): [-Wuser] ! while expanding symbol "warn_unique" -error: Assembly aborted (1 error)! +error: Assembly aborted (3 errors)! diff --git a/test/gfx/rgbgfx_test.cpp b/test/gfx/rgbgfx_test.cpp index 51b697f3..1c0fd1d8 100644 --- a/test/gfx/rgbgfx_test.cpp +++ b/test/gfx/rgbgfx_test.cpp @@ -173,11 +173,6 @@ public: png_set_read_fn(png, this, readData); png_set_sig_bytes(png, pngHeader.size()); - // TODO: png_set_crc_action(png, PNG_CRC_ERROR_QUIT, PNG_CRC_WARN_DISCARD); - - // Skipping chunks we don't use should improve performance - // TODO: png_set_keep_unknown_chunks(png, ...); - // Process all chunks up to but not including the image data png_read_info(png, info); @@ -202,9 +197,7 @@ public: } } - // Set up transformations; to turn everything into RGBA888 - // TODO: it's not necessary to uniformize the pixel data (in theory), and not doing - // so *might* improve performance, and should reduce memory usage. + // Set up transformations to turn everything into RGBA888 for simplicity of handling // Convert grayscale to RGB switch (colorType & ~PNG_COLOR_MASK_ALPHA) {