Refactor warnings and errors (#1728)

* Remove `err` and `warn`, keep `errx` and `warnx`, using them in RGBGFX too

* Separate RGBGFX and RGBLINK warnings/errors from main options

* Separate `report` function into `error` and `fatal` messages

* Implicit newlines for most RGBASM errors

src/gfx/main.cpp

@@ -14,6 +14,7 @@
 #include <string_view>
 #include <vector>
 
+#include "error.hpp"
 #include "extern/getopt.hpp"
 #include "file.hpp"
 #include "platform.hpp"
@@ -22,6 +23,7 @@
 #include "gfx/pal_spec.hpp"
 #include "gfx/process.hpp"
 #include "gfx/reverse.hpp"
+#include "gfx/warning.hpp"
 
 using namespace std::literals::string_view_literals;
 
@@ -37,69 +39,6 @@ static struct LocalOptions {
 	bool reverse;
 } localOptions;
 
-static uintmax_t nbErrors;
-
-[[noreturn]]
-void giveUp() {
-	fprintf(stderr, "Conversion aborted after %ju error%s\n", nbErrors, nbErrors == 1 ? "" : "s");
-	exit(1);
-}
-
-void requireZeroErrors() {
-	if (nbErrors != 0) {
-		giveUp();
-	}
-}
-
-void warning(char const *fmt, ...) {
-	va_list ap;
-
-	fputs("warning: ", stderr);
-	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
-	va_end(ap);
-	putc('\n', stderr);
-}
-
-void error(char const *fmt, ...) {
-	va_list ap;
-
-	fputs("error: ", stderr);
-	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
-	va_end(ap);
-	putc('\n', stderr);
-
-	if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
-		nbErrors++;
-	}
-}
-
-void errorMessage(char const *msg) {
-	fprintf(stderr, "error: %s\n", msg);
-
-	if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
-		nbErrors++;
-	}
-}
-
-[[noreturn]]
-void fatal(char const *fmt, ...) {
-	va_list ap;
-
-	fputs("FATAL: ", stderr);
-	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
-	va_end(ap);
-	putc('\n', stderr);
-
-	if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
-		nbErrors++;
-	}
-
-	giveUp();
-}
-
 void Options::verbosePrint(uint8_t level, char const *fmt, ...) const {
 	// LCOV_EXCL_START
 	if (verbosity >= level) {
@@ -179,6 +118,19 @@ static void printUsage() {
 }
 // LCOV_EXCL_STOP
 
+[[gnu::format(printf, 1, 2), noreturn]]
+static void fatalWithUsage(char const *fmt, ...) {
+	va_list ap;
+	fputs("FATAL: ", stderr);
+	va_start(ap, fmt);
+	vfprintf(stderr, fmt, ap);
+	va_end(ap);
+	putc('\n', stderr);
+
+	printUsage();
+	exit(1);
+}
+
 // Parses a number at the beginning of a string, moving the pointer to skip the parsed characters.
 // Returns the provided errVal on error.
 static uint16_t parseNumber(char *&string, char const *errPrefix, uint16_t errVal = UINT16_MAX) {
@@ -256,19 +208,13 @@ static void skipWhitespace(char *&arg) {
 
 static void registerInput(char const *arg) {
 	if (!options.input.empty()) {
-		fprintf(
-		    stderr,
-		    "FATAL: input image specified more than once! (first \"%s\", then "
-		    "\"%s\")\n",
+		fatalWithUsage(
+		    "Input image specified more than once! (first \"%s\", then \"%s\")",
 		    options.input.c_str(),
 		    arg
 		);
-		printUsage();
-		exit(1);
 	} else if (arg[0] == '\0') { // Empty input path
-		fprintf(stderr, "FATAL: input image path cannot be empty\n");
-		printUsage();
-		exit(1);
+		fatalWithUsage("Input image path cannot be empty");
 	} else {
 		options.input = arg;
 	}
@@ -361,7 +307,7 @@ static char *parseArgv(int argc, char *argv[]) {
 		case 'a':
 			localOptions.autoAttrmap = false;
 			if (!options.attrmap.empty()) {
-				warning("Overriding attrmap file %s", options.attrmap.c_str());
+				warnx("Overriding attrmap file %s", options.attrmap.c_str());
 			}
 			options.attrmap = musl_optarg;
 			break;
@@ -438,7 +384,7 @@ static char *parseArgv(int argc, char *argv[]) {
 			// LCOV_EXCL_STOP
 		case 'i':
 			if (!options.inputTileset.empty()) {
-				warning("Overriding input tileset file %s", options.inputTileset.c_str());
+				warnx("Overriding input tileset file %s", options.inputTileset.c_str());
 			}
 			options.inputTileset = musl_optarg;
 			break;
@@ -548,7 +494,7 @@ static char *parseArgv(int argc, char *argv[]) {
 			break;
 		case 'o':
 			if (!options.output.empty()) {
-				warning("Overriding tile data file %s", options.output.c_str());
+				warnx("Overriding tile data file %s", options.output.c_str());
 			}
 			options.output = musl_optarg;
 			break;
@@ -558,7 +504,7 @@ static char *parseArgv(int argc, char *argv[]) {
 		case 'p':
 			localOptions.autoPalettes = false;
 			if (!options.palettes.empty()) {
-				warning("Overriding palettes file %s", options.palettes.c_str());
+				warnx("Overriding palettes file %s", options.palettes.c_str());
 			}
 			options.palettes = musl_optarg;
 			break;
@@ -568,7 +514,7 @@ static char *parseArgv(int argc, char *argv[]) {
 		case 'q':
 			localOptions.autoPalmap = false;
 			if (!options.palmap.empty()) {
-				warning("Overriding palette map file %s", options.palmap.c_str());
+				warnx("Overriding palette map file %s", options.palmap.c_str());
 			}
 			options.palmap = musl_optarg;
 			break;
@@ -596,7 +542,7 @@ static char *parseArgv(int argc, char *argv[]) {
 		case 't':
 			localOptions.autoTilemap = false;
 			if (!options.tilemap.empty()) {
-				warning("Overriding tilemap file %s", options.tilemap.c_str());
+				warnx("Overriding tilemap file %s", options.tilemap.c_str());
 			}
 			options.tilemap = musl_optarg;
 			break;
@@ -728,13 +674,10 @@ int main(int argc, char *argv[]) {
 		if (autoOptEnabled) {
 			auto &image = localOptions.groupOutputs ? options.output : options.input;
 			if (image.empty()) {
-				fprintf(
-				    stderr,
-				    "FATAL: No %s specified\n",
+				fatalWithUsage(
+				    "No %s specified",
 				    localOptions.groupOutputs ? "output tile data file" : "input image"
 				);
-				printUsage();
-				exit(1);
 			}
 
 			// Manual implementation of std::filesystem::path.replace_extension().
@@ -921,9 +864,7 @@ int main(int argc, char *argv[]) {
 	           && !localOptions.reverse) {
 		processPalettes();
 	} else {
-		fputs("FATAL: No input image specified\n", stderr);
-		printUsage();
-		exit(1);
+		fatalWithUsage("No input image specified");
 	}
 
 	requireZeroErrors();

src/gfx/pal_spec.cpp

@@ -16,10 +16,12 @@
 #include <string_view>
 #include <tuple>
 
+#include "error.hpp"
 #include "helpers.hpp"
 #include "platform.hpp"
 
 #include "gfx/main.hpp"
+#include "gfx/warning.hpp"
 
 using namespace std::string_view_literals;
 
@@ -60,7 +62,7 @@ void parseInlinePalSpec(char const * const rawArg) {
 		assume(ofs <= arg.length());
 		assume(len <= arg.length());
 
-		errorMessage(msg);
+		error("%s", msg); // `format_` and `-Wformat-security` would complain about `error(msg);`
 		fprintf(
 		    stderr,
 		    "In inline palette spec: %s\n"
@@ -286,7 +288,7 @@ static void parsePSPFile(std::filebuf &file) {
 
 	if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes;
 	    *nbColors > nbPalColors) {
-		warning(
+		warnx(
 		    "PSP file contains %" PRIu16 " colors, but there can only be %" PRIu16
 		    "; ignoring extra",
 		    *nbColors,
@@ -368,7 +370,7 @@ static void parseGPLFile(std::filebuf &file) {
 	}
 
 	if (nbColors > maxNbColors) {
-		warning(
+		warnx(
 		    "GPL file contains %" PRIu16 " colors, but there can only be %" PRIu16
 		    "; ignoring extra",
 		    nbColors,
@@ -416,7 +418,7 @@ static void parseHEXFile(std::filebuf &file) {
 	}
 
 	if (nbColors > maxNbColors) {
-		warning(
+		warnx(
 		    "HEX file contains %" PRIu16 " colors, but there can only be %" PRIu16
 		    "; ignoring extra",
 		    nbColors,
@@ -445,7 +447,7 @@ static void parseACTFile(std::filebuf &file) {
 
 	if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes;
 	    nbColors > nbPalColors) {
-		warning(
+		warnx(
 		    "ACT file contains %" PRIu16 " colors, but there can only be %" PRIu16
 		    "; ignoring extra",
 		    nbColors,
@@ -499,7 +501,7 @@ static void parseACOFile(std::filebuf &file) {
 
 	if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes;
 	    nbColors > nbPalColors) {
-		warning(
+		warnx(
 		    "ACO file contains %" PRIu16 " colors, but there can only be %" PRIu16
 		    "; ignoring extra",
 		    nbColors,

src/gfx/process.cpp

@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "defaultinitvec.hpp"
+#include "error.hpp"
 #include "file.hpp"
 #include "helpers.hpp"
 #include "itertools.hpp"
@@ -24,6 +25,7 @@
 #include "gfx/pal_packing.hpp"
 #include "gfx/pal_sorting.hpp"
 #include "gfx/proto_palette.hpp"
+#include "gfx/warning.hpp"
 
 static bool isBgColorTransparent() {
 	return options.bgColor.has_value() && options.bgColor->isTransparent();
@@ -92,7 +94,7 @@ class Png {
 	static void handleWarning(png_structp png, char const *msg) {
 		Png *self = reinterpret_cast<Png *>(png_get_error_ptr(png));
 
-		warning("In input image (\"%s\"): %s", self->c_str(), msg);
+		warnx("In input image (\"%s\"): %s", self->c_str(), msg);
 	}
 
 	static void readData(png_structp png, png_bytep data, size_t length) {
@@ -385,7 +387,7 @@ public:
 				    std::tuple conflicting{color.toCSS(), other->toCSS()};
 				    // Do not report combinations twice
 				    if (std::find(RANGE(conflicts), conflicting) == conflicts.end()) {
-					    warning(
+					    warnx(
 					        "Fusing colors #%08x and #%08x into Game Boy color $%04x [first seen "
 					        "at x: %" PRIu32 ", y: %" PRIu32 "]",
 					        std::get<0>(conflicting),

src/gfx/reverse.cpp

@@ -13,10 +13,12 @@
 #include <vector>
 
 #include "defaultinitvec.hpp"
+#include "error.hpp"
 #include "file.hpp"
 #include "helpers.hpp" // assume
 
 #include "gfx/main.hpp"
+#include "gfx/warning.hpp"
 
 static DefaultInitVec<uint8_t> readInto(std::string const &path) {
 	File file;
@@ -59,7 +61,7 @@ static void pngError(png_structp png, char const *msg) {
 }
 
 static void pngWarning(png_structp png, char const *msg) {
-	warning(
+	warnx(
 	    "While writing reversed image (\"%s\"): %s",
 	    static_cast<char const *>(png_get_error_ptr(png)),
 	    msg
@@ -106,19 +108,19 @@ void reverse() {
 	}
 
 	if (options.allowDedup && options.tilemap.empty()) {
-		warning("Tile deduplication is enabled, but no tilemap is provided?");
+		warnx("Tile deduplication is enabled, but no tilemap is provided?");
 	}
 
 	if (options.useColorCurve) {
-		warning("The color curve is not yet supported in reverse mode...");
+		warnx("The color curve is not yet supported in reverse mode...");
 	}
 
 	if (options.inputSlice.left != 0 || options.inputSlice.top != 0
 	    || options.inputSlice.height != 0) {
-		warning("\"Sliced-off\" pixels are ignored in reverse mode");
+		warnx("\"Sliced-off\" pixels are ignored in reverse mode");
 	}
 	if (options.inputSlice.width != 0 && options.inputSlice.width != options.reversedWidth * 8) {
-		warning(
+		warnx(
 		    "Specified input slice width (%" PRIu16
 		    ") doesn't match provided reversing width (%" PRIu16 " * 8)",
 		    options.inputSlice.width,
@@ -152,7 +154,7 @@ void reverse() {
 		fatal("Cannot generate empty image");
 	}
 	if (mapSize > options.maxNbTiles[0] + options.maxNbTiles[1]) {
-		warning(
+		warnx(
 		    "Total number of tiles (%zu) is more than the limit of %" PRIu16 " + %" PRIu16,
 		    mapSize,
 		    options.maxNbTiles[0],
@@ -234,7 +236,7 @@ void reverse() {
 		} while (nbRead != 0);
 
 		if (palettes.size() > options.nbPalettes) {
-			warning(
+			warnx(
 			    "Read %zu palettes, more than the specified limit of %" PRIu16,
 			    palettes.size(),
 			    options.nbPalettes
@@ -242,7 +244,7 @@ void reverse() {
 		}
 
 		if (options.palSpecType == Options::EXPLICIT && palettes != options.palSpec) {
-			warning("Colors in the palette file do not match those specified with `-c`!");
+			warnx("Colors in the palette file do not match those specified with `-c`!");
 			// This spacing aligns "...versus with `-c`" above the column of `-c` palettes
 			fputs("Colors specified in the palette file:         ...versus with `-c`:\n", stderr);
 			for (size_t i = 0; i < palettes.size() && i < options.palSpec.size(); ++i) {
@@ -263,8 +265,8 @@ void reverse() {
 			palettes[0][i] = grayColors[options.dmgValue(i)];
 		}
 	} else if (options.palSpecType == Options::EMBEDDED) {
-		warning("An embedded palette was requested, but no palette file was specified; ignoring "
-		        "request.");
+		warnx("An embedded palette was requested, but no palette file was specified; ignoring "
+		      "request.");
 	} else if (options.palSpecType == Options::EXPLICIT) {
 		palettes = std::move(options.palSpec); // We won't be using it again.
 	}
@@ -303,7 +305,7 @@ void reverse() {
 
 			if (!tilemap) {
 				if (bank) {
-					warning(
+					warnx(
 					    "Attribute map assigns tile at (%zu, %zu) to bank 1, but no tilemap "
 					    "specified; "
 					    "ignoring the bank bit",

src/gfx/warning.cpp

@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: MIT
+
+#include "gfx/warning.hpp"
+
+#include <limits>
+#include <stdarg.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static uintmax_t nbErrors;
+
+[[noreturn]]
+void giveUp() {
+	fprintf(stderr, "Conversion aborted after %ju error%s\n", nbErrors, nbErrors == 1 ? "" : "s");
+	exit(1);
+}
+
+void requireZeroErrors() {
+	if (nbErrors != 0) {
+		giveUp();
+	}
+}
+
+void error(char const *fmt, ...) {
+	va_list ap;
+	fputs("error: ", stderr);
+	va_start(ap, fmt);
+	vfprintf(stderr, fmt, ap);
+	va_end(ap);
+	putc('\n', stderr);
+
+	if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
+		nbErrors++;
+	}
+}
+
+[[noreturn]]
+void fatal(char const *fmt, ...) {
+	va_list ap;
+	fputs("FATAL: ", stderr);
+	va_start(ap, fmt);
+	vfprintf(stderr, fmt, ap);
+	va_end(ap);
+	putc('\n', stderr);
+
+	if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
+		nbErrors++;
+	}
+
+	giveUp();
+}