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

@@ -47,7 +47,7 @@ public:
 		} else if (mode & std::ios_base::in) {
 			assert(!(mode & std::ios_base::out));
 			_file.emplace<std::streambuf *>(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));
 			}

include/itertools.hpp

@@ -83,7 +83,7 @@ class ZipContainer {
 	std::tuple<Containers...> _containers;
 
 public:
-	ZipContainer(Containers &&...containers)
+	explicit ZipContainer(Containers &&...containers)
 	    : _containers(std::forward<Containers>(containers)...) {}
 
 	auto begin() {

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);

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. */ \
+	/* Evaluate c exactly once in case it has side effects */ \
-	if (i < sizeof(yylval.string)) \
+	if (char v = (c); i < sizeof(yylval.string)) \
 		yylval.string[i++] = v; \
 } while (0)
 

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);
 

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];

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);
+		break;
-		return;
 
 	case WARNING_DEFAULT:
 		unreachable_();
 		// Not reached
-
-	case WARNING_ENABLED:
-		break;
 	}
 
-	printDiag(fmt, args, "warning: ", ": [-W%s]\n    ", flag);
-
 	va_end(args);
 }

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);
 }

src/gfx/main.cpp

@@ -337,6 +337,7 @@ static std::vector<size_t> readAtFile(std::string const &path, std::vector<char>
 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);
+			number = parseNumber(arg, "Bank 0 base tile ID", 0);
-			if (options.baseTileIDs[0] >= 256) {
+			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);
+			number = parseNumber(arg, "Bank 1 base tile ID", 0);
-			if (options.baseTileIDs[1] >= 256) {
+			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':

src/gfx/pal_packing.cpp

@@ -45,7 +45,7 @@ struct ProtoPalAttrs {
 	 */
 	std::vector<bool> 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];
 	}

src/gfx/reverse.cpp

@@ -21,7 +21,7 @@
 
 #include "gfx/main.hpp"
 
-static DefaultInitVec<uint8_t> readInto(std::string path) {
+static DefaultInitVec<uint8_t> 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));

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);
 	}
 }

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;