From 66fd5a7062455982f84343fb30d0410db2e26508 Mon Sep 17 00:00:00 2001 From: Sylvie <35663410+Rangi42@users.noreply.github.com> Date: Thu, 18 Jan 2024 14:47:20 -0500 Subject: [PATCH] Fix some usually disabled compiler warnings (#1286) * Fixes from temporarily re-enabling more compiler warnings * More edits suggested by cppcheck * Fix hanging on append_yylval_string * Fix FOR loop increment --- include/file.hpp | 2 +- include/itertools.hpp | 2 +- src/asm/fstack.cpp | 2 +- src/asm/lexer.cpp | 4 ++-- src/asm/main.cpp | 2 +- src/asm/output.cpp | 2 +- src/asm/warning.cpp | 14 ++++++-------- src/error.cpp | 4 ++-- src/gfx/main.cpp | 21 ++++++++++++++------- src/gfx/pal_packing.cpp | 2 +- src/gfx/reverse.cpp | 2 +- src/link/output.cpp | 4 ++-- src/link/sdas_obj.cpp | 6 +++--- 13 files changed, 36 insertions(+), 31 deletions(-) diff --git a/include/file.hpp b/include/file.hpp index eef3ad52..e1386b13 100644 --- a/include/file.hpp +++ b/include/file.hpp @@ -47,7 +47,7 @@ public: } else if (mode & std::ios_base::in) { assert(!(mode & std::ios_base::out)); _file.emplace(std::cin.rdbuf()); - if (setmode(STDIN_FILENO, mode & std::ios_base::binary ? O_BINARY : O_TEXT) == -1) { + if (setmode(STDIN_FILENO, (mode & std::ios_base::binary) ? O_BINARY : O_TEXT) == -1) { fatal("Failed to set stdin to %s mode: %s", mode & std::ios_base::binary ? "binary" : "text", strerror(errno)); } diff --git a/include/itertools.hpp b/include/itertools.hpp index 1406442c..2e4f8cef 100644 --- a/include/itertools.hpp +++ b/include/itertools.hpp @@ -83,7 +83,7 @@ class ZipContainer { std::tuple _containers; public: - ZipContainer(Containers &&...containers) + explicit ZipContainer(Containers &&...containers) : _containers(std::forward(containers)...) {} auto begin() { diff --git a/src/asm/fstack.cpp b/src/asm/fstack.cpp index 230d09b5..de8efccc 100644 --- a/src/asm/fstack.cpp +++ b/src/asm/fstack.cpp @@ -243,7 +243,7 @@ bool yywrap(void) // Avoid arithmetic overflow runtime error uint32_t forValue = (uint32_t)contextStack->forValue + (uint32_t)contextStack->forStep; - contextStack->forValue = forValue >= 0 ? (int32_t)forValue + contextStack->forValue = forValue <= INT32_MAX ? forValue : -(int32_t)~forValue - 1; struct Symbol *sym = sym_AddVar(contextStack->forName, contextStack->forValue); diff --git a/src/asm/lexer.cpp b/src/asm/lexer.cpp index 9175160c..a485b19a 100644 --- a/src/asm/lexer.cpp +++ b/src/asm/lexer.cpp @@ -1421,8 +1421,8 @@ static char const *readInterpolation(size_t depth) } #define append_yylval_string(c) do { \ - char v = (c); /* Evaluate c exactly once in case it has side effects. */ \ - if (i < sizeof(yylval.string)) \ + /* Evaluate c exactly once in case it has side effects */ \ + if (char v = (c); i < sizeof(yylval.string)) \ yylval.string[i++] = v; \ } while (0) diff --git a/src/asm/main.cpp b/src/asm/main.cpp index 3901a7b8..55361218 100644 --- a/src/asm/main.cpp +++ b/src/asm/main.cpp @@ -323,7 +323,7 @@ int main(int argc, char *argv[]) warnings = false; break; - unsigned int maxValue; + unsigned long maxValue; case 'X': maxValue = strtoul(musl_optarg, &endptr, 0); diff --git a/src/asm/output.cpp b/src/asm/output.cpp index e217c41b..1cbfeea0 100644 --- a/src/asm/output.cpp +++ b/src/asm/output.cpp @@ -265,7 +265,7 @@ static uint32_t getSymbolID(struct Symbol *sym) return sym->ID; } -static void writerpn(uint8_t *rpnexpr, uint32_t *rpnptr, uint8_t *rpn, +static void writerpn(uint8_t *rpnexpr, uint32_t *rpnptr, const uint8_t *rpn, uint32_t rpnlen) { char symName[512]; diff --git a/src/asm/warning.cpp b/src/asm/warning.cpp index 69b19181..9c8f4484 100644 --- a/src/asm/warning.cpp +++ b/src/asm/warning.cpp @@ -372,22 +372,20 @@ void warning(enum WarningID id, char const *fmt, ...) switch (warningState(id)) { case WARNING_DISABLED: - return; + break; + + case WARNING_ENABLED: + printDiag(fmt, args, "warning: ", ": [-W%s]\n ", flag); + break; case WARNING_ERROR: printDiag(fmt, args, "error: ", ": [-Werror=%s]\n ", flag); - va_end(args); - return; + break; case WARNING_DEFAULT: unreachable_(); // Not reached - - case WARNING_ENABLED: - break; } - printDiag(fmt, args, "warning: ", ": [-W%s]\n ", flag); - va_end(args); } diff --git a/src/error.cpp b/src/error.cpp index 2d05facb..2165ee35 100644 --- a/src/error.cpp +++ b/src/error.cpp @@ -32,6 +32,7 @@ static void vwarnx(char const NONNULL(fmt), va_list ap) fprintf(stderr, "error: "); vfprintf(stderr, fmt, ap); fprintf(stderr, ": %s\n", error); + va_end(ap); exit(1); } @@ -40,6 +41,7 @@ static void vwarnx(char const NONNULL(fmt), va_list ap) fprintf(stderr, "error: "); vfprintf(stderr, fmt, ap); putc('\n', stderr); + va_end(ap); exit(1); } @@ -67,7 +69,6 @@ void warnx(char const NONNULL(fmt), ...) va_start(ap, fmt); verr(fmt, ap); - va_end(ap); } [[noreturn]] void errx(char const NONNULL(fmt), ...) @@ -76,5 +77,4 @@ void warnx(char const NONNULL(fmt), ...) va_start(ap, fmt); verrx(fmt, ap); - va_end(ap); } diff --git a/src/gfx/main.cpp b/src/gfx/main.cpp index 33e8f0e7..a92c1164 100644 --- a/src/gfx/main.cpp +++ b/src/gfx/main.cpp @@ -337,6 +337,7 @@ static std::vector readAtFile(std::string const &path, std::vector static char *parseArgv(int argc, char **argv) { for (int ch; (ch = musl_getopt_long_only(argc, argv, optstring, longopts, nullptr)) != -1;) { char *arg = musl_optarg; // Make a copy for scanning + uint16_t number; switch (ch) { case -'A': warning("`--output-attr-map` is deprecated, use `--auto-attr-map` instead"); @@ -351,9 +352,11 @@ static char *parseArgv(int argc, char **argv) { options.attrmap = musl_optarg; break; case 'b': - options.baseTileIDs[0] = parseNumber(arg, "Bank 0 base tile ID", 0); - if (options.baseTileIDs[0] >= 256) { + number = parseNumber(arg, "Bank 0 base tile ID", 0); + if (number >= 256) { error("Bank 0 base tile ID must be below 256"); + } else { + options.baseTileIDs[0] = number; } if (*arg == '\0') { options.baseTileIDs[1] = 0; @@ -367,9 +370,11 @@ static char *parseArgv(int argc, char **argv) { } ++arg; // Skip comma skipWhitespace(arg); - options.baseTileIDs[1] = parseNumber(arg, "Bank 1 base tile ID", 0); - if (options.baseTileIDs[1] >= 256) { + number = parseNumber(arg, "Bank 1 base tile ID", 0); + if (number >= 256) { error("Bank 1 base tile ID must be below 256"); + } else { + options.baseTileIDs[1] = number; } if (*arg != '\0') { error("Base tile IDs must be one or two comma-separated numbers, not \"%s\"", @@ -479,14 +484,16 @@ static char *parseArgv(int argc, char **argv) { } break; case 'n': - options.nbPalettes = parseNumber(arg, "Number of palettes", 256); + number = parseNumber(arg, "Number of palettes", 256); if (*arg != '\0') { error("Number of palettes (-n) must be a valid number, not \"%s\"", musl_optarg); } - if (options.nbPalettes > 256) { + if (number > 256) { error("Number of palettes (-n) must not exceed 256!"); - } else if (options.nbPalettes == 0) { + } else if (number == 0) { error("Number of palettes (-n) may not be 0!"); + } else { + options.nbPalettes = number; } break; case 'O': diff --git a/src/gfx/pal_packing.cpp b/src/gfx/pal_packing.cpp index 41771254..cdf0e3c8 100644 --- a/src/gfx/pal_packing.cpp +++ b/src/gfx/pal_packing.cpp @@ -45,7 +45,7 @@ struct ProtoPalAttrs { */ std::vector bannedPages; - ProtoPalAttrs(size_t index) : protoPalIndex(index) {} + explicit ProtoPalAttrs(size_t index) : protoPalIndex(index) {} bool isBannedFrom(size_t index) const { return index < bannedPages.size() && bannedPages[index]; } diff --git a/src/gfx/reverse.cpp b/src/gfx/reverse.cpp index ea481ba5..4e80a7fa 100644 --- a/src/gfx/reverse.cpp +++ b/src/gfx/reverse.cpp @@ -21,7 +21,7 @@ #include "gfx/main.hpp" -static DefaultInitVec readInto(std::string path) { +static DefaultInitVec readInto(const std::string &path) { File file; if (!file.open(path, std::ios::in | std::ios::binary)) { fatal("Failed to open \"%s\": %s", file.c_str(path), strerror(errno)); diff --git a/src/link/output.cpp b/src/link/output.cpp index d5450453..c6f94b52 100644 --- a/src/link/output.cpp +++ b/src/link/output.cpp @@ -61,7 +61,7 @@ static enum SectionType typeMap[SECTTYPE_INVALID] = { void out_AddSection(struct Section const *section) { - static uint32_t maxNbBanks[] = { + static const uint32_t maxNbBanks[] = { AT(SECTTYPE_WRAM0) 1, AT(SECTTYPE_VRAM) 2, AT(SECTTYPE_ROMX) UINT32_MAX, @@ -544,7 +544,7 @@ static void writeMapSummary(void) nbBanks * sectionTypeInfo[type].size - usedTotal); if (sectionTypeInfo[type].firstBank != sectionTypeInfo[type].lastBank || nbBanks > 1) - fprintf(mapFile, " in %d bank%s", nbBanks, nbBanks == 1 ? "" : "s"); + fprintf(mapFile, " in %u bank%s", nbBanks, nbBanks == 1 ? "" : "s"); putc('\n', mapFile); } } diff --git a/src/link/sdas_obj.cpp b/src/link/sdas_obj.cpp index e13ec520..548039b4 100644 --- a/src/link/sdas_obj.cpp +++ b/src/link/sdas_obj.cpp @@ -569,7 +569,7 @@ void sdobj_ReadFile(struct FileStackNode const *where, FILE *file) { patch->pcSection = section; // No need to fill `pcSectionID`, then patch->pcOffset = patch->offset - 1; // For `jr`s - patch->type = flags & 1 << RELOC_SIZE ? PATCHTYPE_BYTE : PATCHTYPE_WORD; + patch->type = (flags & 1 << RELOC_SIZE) ? PATCHTYPE_BYTE : PATCHTYPE_WORD; uint8_t nbBaseBytes = patch->type == PATCHTYPE_BYTE ? ADDR_SIZE : 2; uint32_t baseValue = 0; @@ -694,7 +694,7 @@ void sdobj_ReadFile(struct FileStackNode const *where, FILE *file) { patch->rpnExpression[patch->rpnSize + 2] = 16 >> 8; patch->rpnExpression[patch->rpnSize + 3] = 16 >> 16; patch->rpnExpression[patch->rpnSize + 4] = 16 >> 24; - patch->rpnExpression[patch->rpnSize + 5] = flags & 1 << RELOC_SIGNED ? RPN_SHR : RPN_USHR; + patch->rpnExpression[patch->rpnSize + 5] = (flags & 1 << RELOC_SIGNED) ? RPN_SHR : RPN_USHR; patch->rpnSize += 5 + 1; } else { if (flags & 1 << RELOC_EXPR16 && flags & 1 << RELOC_WHICHBYTE) { @@ -703,7 +703,7 @@ void sdobj_ReadFile(struct FileStackNode const *where, FILE *file) { patch->rpnExpression[patch->rpnSize + 2] = 8 >> 8; patch->rpnExpression[patch->rpnSize + 3] = 8 >> 16; patch->rpnExpression[patch->rpnSize + 4] = 8 >> 24; - patch->rpnExpression[patch->rpnSize + 5] = flags & 1 << RELOC_SIGNED ? RPN_SHR : RPN_USHR; + patch->rpnExpression[patch->rpnSize + 5] = (flags & 1 << RELOC_SIGNED) ? RPN_SHR : RPN_USHR; patch->rpnSize += 5 + 1; } patch->rpnExpression[patch->rpnSize] = RPN_CONST;