Parse inline palette specs after getting CLI palette size limit (#2054)

include/gfx/main.hpp

@@ -25,7 +25,8 @@ struct Options {
 	std::array<uint8_t, 2> baseTileIDs{0, 0}; // -b
 	enum {
 		NO_SPEC,
-		EXPLICIT,
+		INLINE,
+		EXTERNAL,
 		EMBEDDED,
 		DMG,
 	} palSpecType = NO_SPEC; // -c
@@ -59,6 +60,8 @@ struct Options {
 
 	uint16_t maxNbColors() const { return nbColorsPerPal * nbPalettes; }
 
+	bool hasExplicitPalSpec() const { return palSpecType == INLINE || palSpecType == EXTERNAL; }
+
 	uint8_t dmgColors[4] = {};
 	uint8_t dmgValue(uint8_t i) const {
 		assume(i < 4);

include/gfx/pal_spec.hpp

@@ -8,8 +8,6 @@
 void parseInlinePalSpec(char const * const rawArg);
 void parseExternalPalSpec(char const *arg);
 void parseDmgPalSpec(char const * const rawArg);
-void parseDmgPalSpec(uint8_t palSpecDmg);
-
 void parseBackgroundPalSpec(char const *arg);
 
 #endif // RGBDS_GFX_PAL_SPEC_HPP

src/gfx/main.cpp

@@ -36,13 +36,13 @@ Options options;
 
 // Flags which must be processed after the option parsing finishes
 static struct LocalOptions {
-	std::optional<std::string> externalPalSpec; // -c
+	std::optional<std::string> palSpec; // -c
-	bool autoAttrmap;                           // -A
+	bool autoAttrmap;                   // -A
-	bool autoTilemap;                           // -T
+	bool autoTilemap;                   // -T
-	bool autoPalettes;                          // -P
+	bool autoPalettes;                  // -P
-	bool autoPalmap;                            // -Q
+	bool autoPalmap;                    // -Q
-	bool groupOutputs;                          // -O
+	bool groupOutputs;                  // -O
-	bool reverse;                               // -r
+	bool reverse;                       // -r
 
 	bool autoAny() const { return autoAttrmap || autoTilemap || autoPalettes || autoPalmap; }
 } localOptions;
@@ -193,10 +193,10 @@ static void parseArg(int ch, char *arg) {
 		break;
 
 	case 'c':
-		localOptions.externalPalSpec = std::nullopt; // Allow overriding a previous pal spec
+		localOptions.palSpec = std::nullopt; // Allow overriding a previous pal spec
 		if (arg[0] == '#') {
-			options.palSpecType = Options::EXPLICIT;
+			options.palSpecType = Options::INLINE;
-			parseInlinePalSpec(arg);
+			localOptions.palSpec = arg;
 		} else if (strcasecmp(arg, "embedded") == 0) {
 			// Use PLTE, error out if missing
 			options.palSpecType = Options::EMBEDDED;
@@ -204,13 +204,13 @@ static void parseArg(int ch, char *arg) {
 			options.palSpecType = Options::NO_SPEC;
 		} else if (strcasecmp(arg, "dmg") == 0) {
 			options.palSpecType = Options::DMG;
-			parseDmgPalSpec(0xE4); // Same darkest-first order as `sortGrayscale`
+			localOptions.palSpec = "e4"; // Same darkest-first order as `sortGrayscale`
 		} else if (strncasecmp(arg, "dmg=", literal_strlen("dmg=")) == 0) {
 			options.palSpecType = Options::DMG;
-			parseDmgPalSpec(&arg[literal_strlen("dmg=")]);
+			localOptions.palSpec = &arg[literal_strlen("dmg=")];
 		} else {
-			options.palSpecType = Options::EXPLICIT;
+			options.palSpecType = Options::EXTERNAL;
-			localOptions.externalPalSpec = arg;
+			localOptions.palSpec = arg;
 		}
 		break;
 
@@ -514,24 +514,13 @@ static void verboseOutputConfig() {
 	// -s/--palette-size
 	fprintf(stderr, "\tPalettes contain %" PRIu8 " colors\n", options.nbColorsPerPal);
 	// -c/--colors
-	if (options.palSpecType == Options::NO_SPEC) {
+	switch (options.palSpecType) {
+	case Options::NO_SPEC:
 		fputs("\tAutomatic palette generation\n", stderr);
-	} else {
+		break;
-		fprintf(stderr, "\t%s palette spec\n", [] {
+	case Options::INLINE:
-			switch (options.palSpecType) {
+	case Options::EXTERNAL:
-			case Options::EXPLICIT:
+		fputs("\tExplicit palette spec\n\t[\n", stderr);
-				return "Explicit";
-			case Options::EMBEDDED:
-				return "Embedded";
-			case Options::DMG:
-				return "DMG";
-			default:
-				return "???";
-			}
-		}());
-	}
-	if (options.palSpecType == Options::EXPLICIT) {
-		fputs("\t[\n", stderr);
 		for (auto const &pal : options.palSpec) {
 			fputs("\t\t", stderr);
 			for (auto const &color : pal) {
@@ -544,6 +533,13 @@ static void verboseOutputConfig() {
 			putc('\n', stderr);
 		}
 		fputs("\t]\n", stderr);
+		break;
+	case Options::EMBEDDED:
+		fputs("\tEmbedded palette spec from PNG indexed PLTE chunk\n", stderr);
+		break;
+	case Options::DMG:
+		fprintf(stderr, "\tDMG palette spec $%02" PRIx8 "\n", options.palSpecDmg);
+		break;
 	}
 	// -L/--slice
 	if (options.inputSlice.width || options.inputSlice.height || options.inputSlice.left
@@ -671,9 +667,24 @@ int main(int argc, char *argv[]) {
 	autoOutPath(localOptions.autoPalettes, options.palettes, ".pal");
 	autoOutPath(localOptions.autoPalmap, options.palmap, ".palmap");
 
-	// Execute deferred external pal spec parsing, now that all other params are known
+	// Execute deferred pal spec parsing, now that all other params are known
-	if (localOptions.externalPalSpec) {
+	switch (options.palSpecType) {
-		parseExternalPalSpec(localOptions.externalPalSpec->c_str());
+	case Options::NO_SPEC:
+	case Options::EMBEDDED:
+		assume(!localOptions.palSpec);
+		break;
+	case Options::INLINE:
+		assume(localOptions.palSpec);
+		parseInlinePalSpec(localOptions.palSpec->c_str());
+		break;
+	case Options::EXTERNAL:
+		assume(localOptions.palSpec);
+		parseExternalPalSpec(localOptions.palSpec->c_str());
+		break;
+	case Options::DMG:
+		assume(localOptions.palSpec);
+		parseDmgPalSpec(localOptions.palSpec->c_str());
+		break;
 	}
 
 	verboseDo(VERB_CONFIG, verboseOutputConfig);
@@ -687,8 +698,7 @@ int main(int argc, char *argv[]) {
 		} else {
 			process();
 		}
-	} else if (!options.palettes.empty() && options.palSpecType == Options::EXPLICIT
+	} else if (!options.palettes.empty() && options.hasExplicitPalSpec() && !localOptions.reverse) {
-	           && !localOptions.reverse) {
 		processPalettes();
 	} else {
 		usage.printAndExit("No input file specified (pass \"-\" to read from standard input)");

src/gfx/pal_spec.cpp

@@ -29,6 +29,7 @@
 #include "gfx/rgba.hpp"
 #include "gfx/warning.hpp"
 
+using namespace std::literals;
 using namespace std::string_view_literals;
 
 static char const *hexDigits = "0123456789ABCDEFabcdef";
@@ -131,8 +132,14 @@ void parseInlinePalSpec(char const * const rawArg) {
 			if (n == arg.length()) {
 				break;
 			} else if (arg[n] != ';' && arg[n] != ':') {
-				if (nbColors == 4) {
+				if (nbColors == options.nbColorsPerPal) {
-					parseError(n, 1, "Each palette can only contain up to 4 colors");
+					// `parseError` cannot take variadic arguments, since `format_` and
+					// `-Wformat-security` would complain about passing a template parameter pack
+					// to the C-style variadic `error` function, so we format the error message
+					// before passing it to `parseError`.
+					std::string msg = "Each palette can only contain up to "s
+					                  + std::to_string(options.nbColorsPerPal) + " colors";
+					parseError(n, 1, msg.c_str());
 					return;
 				}
 				break;
@@ -664,11 +671,7 @@ void parseDmgPalSpec(char const * const rawArg) {
 		return;
 	}
 
-	parseDmgPalSpec(toHex(arg[0], arg[1]));
+	options.palSpecDmg = toHex(arg[0], arg[1]);
-}
-
-void parseDmgPalSpec(uint8_t palSpecDmg) {
-	options.palSpecDmg = palSpecDmg;
 
 	// Map gray shades to their DMG color indexes for fast lookup by `Rgba::grayIndex`
 	for (uint8_t i = 0; i < 4; ++i) {

src/gfx/reverse.cpp

@@ -249,7 +249,7 @@ void reverse() {
 			);
 		}
 
-		if (options.palSpecType == Options::EXPLICIT && palettes != options.palSpec) {
+		if (options.hasExplicitPalSpec() && palettes != options.palSpec) {
 			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);
@@ -273,7 +273,7 @@ void reverse() {
 	} else if (options.palSpecType == Options::EMBEDDED) {
 		warnx("An embedded palette was requested, but no palette file was specified; ignoring "
 		      "request");
-	} else if (options.palSpecType == Options::EXPLICIT) {
+	} else if (options.hasExplicitPalSpec()) {
 		palettes = std::move(options.palSpec); // We won't be using it again.
 	}
 

test/gfx/long_inline_pal_spec.err

@@ -0,0 +1,4 @@
+error: Each palette can only contain up to 2 colors
+In inline palette spec: "#ffffff,#888888,#000000"
+                                         ^
+Conversion aborted after 1 error

test/gfx/long_inline_pal_spec.flags

@@ -0,0 +1 @@
+-c #ffffff,#888888,#000000 -s 2