Implement warning diagnostic flags for RGBGFX (#1738)

contrib/bash_compl/_rgbgfx.bash

@@ -36,6 +36,7 @@ _rgbgfx_completions() {
 		[s]="palette-size:unk"
 		[t]="tilemap:glob-*.tilemap"
 		[T]="auto-tilemap:normal"
+		[W]="warning:warning"
 		[x]="trim-end:unk"
 	)
 	# Parse command-line up to current word
@@ -152,6 +153,14 @@ _rgbgfx_completions() {
 	case "$state" in
 		unk) # Return with no replies: no idea what to complete!
 			;;
+		warning)
+			mapfile -t COMPREPLY < <(compgen -W "
+				embedded
+				trim-nonempty
+				all
+				everything
+				error" -P "${cur_word:0:$optlen}" -- "${cur_word:$optlen}")
+			;;
 		normal) # Acts like a glob...
 			state="glob-*.png"
 			;&

contrib/zsh_compl/_rgbgfx

@@ -9,6 +9,20 @@ _depths() {
 	_describe 'bit depth' depths
 }
 
+_rgbgfx_warnings() {
+	local warnings=(
+		'error:Turn all warnings into errors'
+
+		'all:Enable most warning messages'
+		'everything:Enable literally everything'
+
+		'embedded:Warn when using embedded PLTE without "-c embedded"'
+		'trim-nonempty:Warn when "-x" trims nonempty tiles'
+	)
+	# TODO: handle `no-` and `error=` somehow?
+	_describe warning warnings
+}
+
 local args=(
 	# Arguments are listed here in the same order as in the manual, except for the version and help
 	'(- : * options)'{-V,--version}'[Print version number and exit]'
@@ -23,6 +37,7 @@ local args=(
 	'(-t --tilemap -T --auto-tilemap)'{-T,--auto-tilemap}'[Shortcut for -t <file>.tilemap]'
 	'(-u --unique-tiles)'{-u,--unique-tiles}'[Eliminate redundant tiles]'
 	{-v,--verbose}'[Enable verbose output]'
+	-w'[Disable all warnings]'
 	'(-X --mirror-x)'{-X,--mirror-x}'[Eliminate horizontally mirrored tiles from output]'
 	'(-Y --mirror-y)'{-Y,--mirror-y}'[Eliminate vertically mirrored tiles from output]'
 	'(-Z --columns)'{-Z,--columns}'[Read the image in column-major order]'
@@ -42,6 +57,7 @@ local args=(
 	'(-r --reverse)'{-r,--reverse}'+[Yield an image from binary data]:image width (in tiles):'
 	'(-s --palette-size)'{-s,--palette-size}'+[Limit palette size]:palette size:'
 	'(-t --tilemap -T --auto-tilemap)'{-t,--tilemap}'+[Generate a map of tile indices]:tilemap file:_files'
+	'(-W --warning)'{-W,--warning}'+[Toggle warning flags]:warning flag:_rgbgfx_warnings'
 	'(-x --trim-end)'{-x,--trim-end}'+[Trim end of output by this many tiles]:tile count:'
 
 	":input png file:_files -g '*.png'"

include/gfx/warning.hpp

@@ -3,6 +3,29 @@
 #ifndef RGBDS_GFX_WARNING_HPP
 #define RGBDS_GFX_WARNING_HPP
 
+#include "diagnostics.hpp"
+
+enum WarningLevel {
+	LEVEL_DEFAULT,    // Warnings that are enabled by default
+	LEVEL_ALL,        // Warnings that probably indicate an error
+	LEVEL_EVERYTHING, // Literally every warning
+};
+
+enum WarningID {
+	WARNING_EMBEDDED,      // Using an embedded PNG palette without '-c embedded'
+	WARNING_TRIM_NONEMPTY, // '-x' trims nonempty tiles
+
+	NB_PLAIN_WARNINGS,
+
+	NB_WARNINGS = NB_PLAIN_WARNINGS,
+};
+
+extern Diagnostics<WarningLevel, WarningID> warnings;
+
+// Warns the user about problems that don't prevent valid graphics conversion
+[[gnu::format(printf, 2, 3)]]
+void warning(WarningID id, char const *fmt, ...);
+
 // Prints the error count, and exits with failure
 [[noreturn]]
 void giveUp();

man/rgbgfx.1

@@ -10,7 +10,7 @@
 .Nd Game Boy graphics converter
 .Sh SYNOPSIS
 .Nm
-.Op Fl CmhOuVXYZ
+.Op Fl CmhOuVwXYZ
 .Op Fl v Op Fl v No ...
 .Op Fl a Ar attrmap | Fl A
 .Op Fl b Ar base_ids
@@ -27,6 +27,7 @@
 .Op Fl r Ar width
 .Op Fl s Ar nb_colors
 .Op Fl t Ar tilemap | Fl T
+.Op Fl W Ar warning
 .Op Fl x Ar quantity
 .Ar file
 .Sh DESCRIPTION
@@ -393,6 +394,17 @@ Some internal debug printing is enabled.
 The verbosity level does not go past 6.
 .Pp
 Note that verbose output is only intended to be consumed by humans, and may change without notice between RGBDS releases; relying on those for scripts is not advised.
+.It Fl W Ar warning , Fl \-warning Ar warning
+Set warning flag
+.Ar warning .
+A warning message will be printed if
+.Ar warning
+is an unknown warning flag.
+See the
+.Sx DIAGNOSTICS
+section for a list of warnings.
+.It Fl w
+Disable all warning output, even when turned into errors.
 .It Fl X , Fl \-mirror-x
 Deduplicate tiles that are horizontally symmetrical mirror images of each other across the X axis.
 Implies
@@ -708,6 +720,68 @@ assume that tiles were not deduplicated, and should be laid out in the order the
 .Nm
 assumes that no tiles were mirrored.
 .El
+.Sh DIAGNOSTICS
+Warnings are diagnostic messages that indicate possibly erroneous behavior that does not necessarily compromise the conversion process.
+The following options alter the way warnings are processed.
+.Bl -tag -width Ds
+.It Fl Werror
+Make all warnings into errors.
+This can be negated as
+.Fl Wno-error
+to prevent turning all warnings into errors.
+.It Fl Werror=
+Make the specified warning or meta warning into an error.
+A warning's name is appended
+.Pq example: Fl Werror=embedded ,
+and this warning is implicitly enabled and turned into an error.
+This can be negated as
+.Fl Wno-error=
+to prevent turning a specified warning into an error, even if
+.Fl Werror
+is in effect.
+.El
+.Pp
+The following warnings are
+.Dq meta
+warnings, that enable a collection of other warnings.
+If a specific warning is toggled via a meta flag and a specific one, the more specific one takes priority.
+The position on the command-line acts as a tie breaker, the last one taking effect.
+.Bl -tag -width Ds
+.It Fl Wall
+This enables warnings that are likely to indicate an error or undesired behavior, and that can easily be fixed.
+.It Fl Weverything
+Enables literally every warning.
+.El
+.Pp
+The following warnings are actual warning flags; with each description, the corresponding warning flag is included.
+Note that each of these flag also has a negation (for example,
+.Fl Wtrim-nonempty
+enables the warning that
+.Fl Wno-trim-nonempty
+disables; and
+.Fl Wall
+enables every warning that
+.Fl Wno-all
+disables).
+Only the non-default flag is listed here.
+Ignoring the
+.Dq no-
+prefix, entries are listed alphabetically.
+.Bl -tag -width Ds
+.It Fl Wembedded
+Warn when a generated palette is sorted according to the input PNG's embedded palette but
+.Fl c Cm embedded
+was not provided.
+This warning is enabled by
+.Fl Weverything .
+.It Fl Wtrim-nonempty
+Warn when
+.Fl x
+trims a nonempty tile.
+An "empty" tile uses entirely color 0 of its palette.
+This warning is enabled by
+.Fl Wall .
+.El
 .Sh EXAMPLES
 The following will only validate the
 .Ql tileset.png

src/gfx/main.cpp

@@ -52,7 +52,7 @@ void Options::verbosePrint(uint8_t level, char const *fmt, ...) const {
 }
 
 // Short options
-static char const *optstring = "-Aa:B:b:Cc:d:hi:L:l:mN:n:Oo:Pp:Qq:r:s:Tt:U:uVvXx:YZ";
+static char const *optstring = "-Aa:B:b:Cc:d:hi:L:l:mN:n:Oo:Pp:Qq:r:s:Tt:U:uVvW:wXx:YZ";
 
 // Equivalent long options
 // Please keep in the same order as short opts.
@@ -90,6 +90,7 @@ static option const longopts[] = {
     {"unique-tiles",     no_argument,       nullptr, 'u'},
     {"version",          no_argument,       nullptr, 'V'},
     {"verbose",          no_argument,       nullptr, 'v'},
+    {"warning",          required_argument, nullptr, 'W'},
     {"mirror-x",         no_argument,       nullptr, 'X'},
     {"trim-end",         required_argument, nullptr, 'x'},
     {"mirror-y",         no_argument,       nullptr, 'Y'},
@@ -558,6 +559,12 @@ static char *parseArgv(int argc, char *argv[]) {
 			}
 			break;
 			// LCOV_EXCL_STOP
+		case 'W':
+			warnings.processWarningFlag(musl_optarg);
+			break;
+		case 'w':
+			warnings.state.warningsEnabled = false;
+			break;
 		case 'x':
 			options.trim = parseNumber(arg, "Number of tiles to trim", 0);
 			if (*arg != '\0') {

src/gfx/process.cpp

@@ -615,6 +615,10 @@ static std::tuple<std::vector<size_t>, std::vector<Palette>>
 		sortGrayscale(palettes, png.getColors().raw());
 	} else if (auto [embPalSize, embPalRGB, embPalAlphaSize, embPalAlpha] = png.getEmbeddedPal();
 	           embPalRGB != nullptr) {
+		warning(
+		    WARNING_EMBEDDED,
+		    "Sorting palette colors by PNG's embedded PLTE chunk without '-c/--colors embedded'"
+		);
 		sortIndexed(palettes, embPalSize, embPalRGB, embPalAlphaSize, embPalAlpha);
 	} else if (png.isSuitableForGrayscale()) {
 		sortGrayscale(palettes, png.getColors().raw());
@@ -870,32 +874,41 @@ static void outputUnoptimizedTileData(
 	uint16_t widthTiles = options.inputSlice.width ? options.inputSlice.width : png.getWidth() / 8;
 	uint16_t heightTiles =
 	    options.inputSlice.height ? options.inputSlice.height : png.getHeight() / 8;
-	uint64_t remainingTiles = widthTiles * heightTiles;
+	uint64_t nbTiles = widthTiles * heightTiles;
-	if (remainingTiles <= options.trim) {
+	uint64_t nbKeptTiles = nbTiles > options.trim ? nbTiles - options.trim : 0;
-		return;
+	uint64_t tileIdx = 0;
-	}
-	remainingTiles -= options.trim;
 
 	for (auto [tile, attr] : zip(png.visitAsTiles(), attrmap)) {
 		// Do not emit fully-background tiles.
 		if (!attr.isBackgroundTile()) {
 			// If the tile is fully transparent, this defaults to palette 0.
 			Palette const &palette = palettes[attr.getPalID(mappings)];
+
+			bool empty = true;
 			for (uint32_t y = 0; y < 8; ++y) {
 				uint16_t bitplanes = TileData::rowBitplanes(tile, palette, y);
-				output->sputc(bitplanes & 0xFF);
+				if (bitplanes != 0) {
-				if (options.bitDepth == 2) {
+					empty = false;
-					output->sputc(bitplanes >> 8);
+				}
+				if (tileIdx < nbKeptTiles) {
+					output->sputc(bitplanes & 0xFF);
+					if (options.bitDepth == 2) {
+						output->sputc(bitplanes >> 8);
+					}
 				}
 			}
-		}
 
-		--remainingTiles;
+			if (!empty && tileIdx >= nbKeptTiles) {
-		if (remainingTiles == 0) {
+				warning(
-			break;
+				    WARNING_TRIM_NONEMPTY,
+				    "Trimming a nonempty tile (configure with '-x/--trim-end'"
+				);
+				break; // Don't repeat the warning for subsequent tiles
+			}
 		}
+		++tileIdx;
 	}
-	assume(remainingTiles == 0);
+	assume(nbKeptTiles <= tileIdx && tileIdx <= nbTiles);
 }
 
 static void outputUnoptimizedMaps(

src/gfx/warning.cpp

@@ -10,6 +10,21 @@
 
 static uintmax_t nbErrors;
 
+// clang-format off: nested initializers
+Diagnostics<WarningLevel, WarningID> warnings = {
+    .metaWarnings = {
+        {"all",           LEVEL_ALL       },
+        {"everything",    LEVEL_EVERYTHING},
+    },
+    .warningFlags = {
+        {"embedded",      LEVEL_EVERYTHING},
+        {"trim-nonempty", LEVEL_ALL       },
+    },
+    .paramWarnings = {},
+    .state = DiagnosticsState<WarningID>(),
+};
+// clang-format on
+
 [[noreturn]]
 void giveUp() {
 	fprintf(stderr, "Conversion aborted after %ju error%s\n", nbErrors, nbErrors == 1 ? "" : "s");
@@ -50,3 +65,33 @@ void fatal(char const *fmt, ...) {
 
 	giveUp();
 }
+
+void warning(WarningID id, char const *fmt, ...) {
+	char const *flag = warnings.warningFlags[id].name;
+	va_list ap;
+
+	switch (warnings.getWarningBehavior(id)) {
+	case WarningBehavior::DISABLED:
+		break;
+
+	case WarningBehavior::ENABLED:
+		fprintf(stderr, "warning: [-W%s]\n    ", flag);
+		va_start(ap, fmt);
+		vfprintf(stderr, fmt, ap);
+		va_end(ap);
+		putc('\n', stderr);
+		break;
+
+	case WarningBehavior::ERROR:
+		fprintf(stderr, "error: [-Werror=%s]\n    ", flag);
+		va_start(ap, fmt);
+		vfprintf(stderr, fmt, ap);
+		va_end(ap);
+		putc('\n', stderr);
+
+		if (nbErrors != std::numeric_limits<decltype(nbErrors)>::max()) {
+			nbErrors++;
+		}
+		break;
+	}
+}

test/gfx/warn_embedded.err

@@ -0,0 +1,3 @@
+error: [-Werror=embedded]
+    Sorting palette colors by PNG's embedded PLTE chunk without '-c/--colors embedded'
+Conversion aborted after 1 error

test/gfx/warn_embedded.flags

@@ -0,0 +1 @@
+-Werror=embedded

test/gfx/warn_trim_nonempty.err

@@ -0,0 +1,2 @@
+warning: [-Wtrim-nonempty]
+    Trimming a nonempty tile (configure with '-x/--trim-end'

test/gfx/warn_trim_nonempty.flags

@@ -0,0 +1,2 @@
+-Wtrim-nonempty
+-x 2