Reduce deep nesting some more, including larger refactors to assign.cpp

src/asm/charmap.cpp

@@ -31,16 +31,17 @@ struct CharmapNode {
 struct Charmap {
 	std::string name;
 	std::vector<CharmapNode> nodes; // first node is reserved for the root node
+};
 
 // Traverse the trie depth-first to derive the character mappings in definition order
 template<typename F>
-	bool forEachChar(F callback) const {
+bool forEachChar(Charmap const &charmap, F callback) {
 	// clang-format off: nested initializers
 	for (std::stack<std::pair<size_t, std::string>> prefixes({{0, ""}}); !prefixes.empty();) {
 		// clang-format on
 		auto [nodeIdx, mapping] = std::move(prefixes.top());
 		prefixes.pop();
-			CharmapNode const &node = nodes[nodeIdx];
+		CharmapNode const &node = charmap.nodes[nodeIdx];
 		if (node.isTerminal() && !callback(nodeIdx, mapping)) {
 			return false;
 		}
@@ -52,7 +53,6 @@ struct Charmap {
 	}
 	return true;
 }
-};
 
 static std::deque<Charmap> charmapList;
 static std::unordered_map<std::string, size_t> charmapMap; // Indexes into `charmapList`
@@ -66,7 +66,7 @@ bool charmap_ForEach(
 ) {
 	for (Charmap const &charmap : charmapList) {
 		std::map<size_t, std::string> mappings;
-		charmap.forEachChar([&mappings](size_t nodeIdx, std::string const &mapping) {
+		forEachChar(charmap, [&mappings](size_t nodeIdx, std::string const &mapping) {
 			mappings[nodeIdx] = mapping;
 			return true;
 		});
@@ -305,7 +305,7 @@ size_t charmap_ConvertNext(std::string_view &input, std::vector<int32_t> *output
 std::string charmap_Reverse(std::vector<int32_t> const &value, bool &unique) {
 	Charmap const &charmap = *currentCharmap;
 	std::string revMapping;
-	unique = charmap.forEachChar([&](size_t nodeIdx, std::string const &mapping) {
+	unique = forEachChar(charmap, [&](size_t nodeIdx, std::string const &mapping) {
 		if (charmap.nodes[nodeIdx].value == value) {
 			if (revMapping.empty()) {
 				revMapping = mapping;

src/asm/output.cpp

@@ -144,12 +144,19 @@ static void registerUnregisteredSymbol(Symbol &sym) {
 }
 
 static void writeRpn(std::vector<uint8_t> &rpnexpr, std::vector<uint8_t> const &rpn) {
-	std::string symName;
 	size_t rpnptr = 0;
 
 	for (size_t offset = 0; offset < rpn.size();) {
 		uint8_t rpndata = rpn[offset++];
 
+		auto getSymName = [&](){
+			std::string symName;
+			for (uint8_t c; (c = rpn[offset++]) != 0;) {
+				symName += c;
+			}
+			return symName;
+		};
+
 		switch (rpndata) {
 			Symbol *sym;
 			uint32_t value;
@@ -164,17 +171,8 @@ static void writeRpn(std::vector<uint8_t> &rpnexpr, std::vector<uint8_t> const &
 			break;
 
 		case RPN_SYM:
-			symName.clear();
-			for (;;) {
-				uint8_t c = rpn[offset++];
-				if (c == 0) {
-					break;
-				}
-				symName += c;
-			}
-
 			// The symbol name is always written expanded
-			sym = sym_FindExactSymbol(symName);
+			sym = sym_FindExactSymbol(getSymName());
 			if (sym->isConstant()) {
 				rpnexpr[rpnptr++] = RPN_CONST;
 				value = sym->getConstantValue();
@@ -191,17 +189,8 @@ static void writeRpn(std::vector<uint8_t> &rpnexpr, std::vector<uint8_t> const &
 			break;
 
 		case RPN_BANK_SYM:
-			symName.clear();
-			for (;;) {
-				uint8_t c = rpn[offset++];
-				if (c == 0) {
-					break;
-				}
-				symName += c;
-			}
-
 			// The symbol name is always written expanded
-			sym = sym_FindExactSymbol(symName);
+			sym = sym_FindExactSymbol(getSymName());
 			registerUnregisteredSymbol(*sym); // Ensure that `sym->ID` is set
 			value = sym->ID;
 

src/gfx/process.cpp

@@ -364,8 +364,7 @@ public:
 		std::vector<uint32_t> indeterminates;
 
 		// Assign a color to the given position, and register it in the image palette as well
-		auto assignColor =
+		auto assignColor = [&](png_uint_32 x, png_uint_32 y, Rgba &&color) {
-		    [this, &conflicts, &indeterminates](png_uint_32 x, png_uint_32 y, Rgba &&color) {
 			if (!color.isTransparent() && !color.isOpaque()) {
 				uint32_t css = color.toCSS();
 				if (std::find(RANGE(indeterminates), css) == indeterminates.end()) {
@@ -403,12 +402,12 @@ public:
 
 		if (interlaceType == PNG_INTERLACE_NONE) {
 			for (png_uint_32 y = 0; y < height; ++y) {
-				png_read_row(png, row.data(), nullptr);
+				png_bytep ptr = row.data();
+				png_read_row(png, ptr, nullptr);
 
 				for (png_uint_32 x = 0; x < width; ++x) {
-					assignColor(
+					assignColor(x, y, Rgba(ptr[0], ptr[1], ptr[2], ptr[3]));
-					    x, y, Rgba(row[x * 4], row[x * 4 + 1], row[x * 4 + 2], row[x * 4 + 3])
+					ptr += 4;
-					);
 				}
 			}
 		} else {

src/gfx/reverse.cpp

@@ -211,10 +211,17 @@ void reverse() {
 
 		palettes.clear();
 		std::array<uint8_t, sizeof(uint16_t) * 4> buf; // 4 colors
-		size_t nbRead;
+		for (;;) {
-		do {
+			if (size_t nbRead = file->sgetn(reinterpret_cast<char *>(buf.data()), buf.size());
-			nbRead = file->sgetn(reinterpret_cast<char *>(buf.data()), buf.size());
+			    nbRead == 0) {
-			if (nbRead == buf.size()) {
+				break;
+			} else if (nbRead != buf.size()) {
+				fatal(
+				    "Palette data size (%zu) is not a multiple of %zu bytes!\n",
+				    palettes.size() * buf.size() + nbRead,
+				    buf.size()
+				);
+			}
 			// Expand the colors
 			auto &palette = palettes.emplace_back();
 			std::generate(
@@ -225,14 +232,7 @@ void reverse() {
 				    return Rgba::fromCGBColor(buf[i - 2] + (buf[i - 1] << 8));
 			    }
 			);
-			} else if (nbRead != 0) {
-				fatal(
-				    "Palette data size (%zu) is not a multiple of %zu bytes!\n",
-				    palettes.size() * buf.size() + nbRead,
-				    buf.size()
-				);
 		}
-		} while (nbRead != 0);
 
 		if (palettes.size() > options.nbPalettes) {
 			warnx(

src/link/assign.cpp

@@ -34,8 +34,6 @@ struct FreeSpace {
 // Table of free space for each bank
 static std::vector<std::deque<FreeSpace>> memory[SECTTYPE_INVALID];
 
-static uint64_t nbSectionsToAssign;
-
 // Init the free space-modelling structs
 static void initFreeSpace() {
 	for (SectionType type : EnumSeq(SECTTYPE_INVALID)) {
@@ -58,8 +56,6 @@ static void assignSection(Section &section, MemoryLocation const &location) {
 		next->bank = location.bank;
 	}
 
-	--nbSectionsToAssign;
-
 	out_AddSection(section);
 }
 
@@ -84,15 +80,13 @@ static bool isLocationSuitable(
 	return location.address + section.size <= freeSpace.address + freeSpace.size;
 }
 
-// Returns a suitable free space index into `memory[section->type]` at which to place the given
+static MemoryLocation getStartLocation(Section const &section) {
-// section, or -1 if none was found.
-static ssize_t getPlacement(Section const &section, MemoryLocation &location) {
-	SectionTypeInfo const &typeInfo = sectionTypeInfo[section.type];
-
 	static uint16_t curScrambleROM = 0;
 	static uint8_t curScrambleWRAM = 0;
 	static int8_t curScrambleSRAM = 0;
 
+	MemoryLocation location;
+
 	// Determine which bank we should start searching in
 	if (section.isBankFixed) {
 		location.bank = section.bank;
@@ -112,10 +106,17 @@ static ssize_t getPlacement(Section const &section, MemoryLocation &location) {
 		}
 		location.bank = curScrambleSRAM--;
 	} else {
-		location.bank = typeInfo.firstBank;
+		location.bank = sectionTypeInfo[section.type].firstBank;
 	}
 
-	for (;;) {
+	return location;
+}
+
+// Returns a suitable free space index into `memory[section->type]` at which to place the given
+// section, or -1 if none was found.
+static ssize_t getPlacement(Section const &section, MemoryLocation &location) {
+	SectionTypeInfo const &typeInfo = sectionTypeInfo[section.type];
+
 	// Switch to the beginning of the next bank
 	std::deque<FreeSpace> &bankMem = memory[section.type][location.bank - typeInfo.firstBank];
 	size_t spaceIdx = 0;
@@ -133,9 +134,8 @@ static ssize_t getPlacement(Section const &section, MemoryLocation &location) {
 
 		// Go to the next *possible* location
 		if (section.isAddressFixed) {
-				// If the address is fixed, there can be only
+			// If the address is fixed, there can be only one candidate block per bank;
-				// one candidate block per bank; if we already
+			// if we already reached it, give up.
-				// reached it, give up.
 			if (location.address < section.org) {
 				location.address = section.org;
 			} else {
@@ -201,7 +201,55 @@ static ssize_t getPlacement(Section const &section, MemoryLocation &location) {
 	} else {
 		return -1;
 	}
+
+	return getPlacement(section, location); // Tail recursion
 }
+
+static std::string getSectionDescription(Section const &section) {
+	std::string where;
+
+	char bank[8], addr[8], mask[8], offset[8];
+	if (section.isBankFixed && nbbanks(section.type) != 1) {
+		snprintf(bank, sizeof(bank), "%02" PRIx32, section.bank);
+	}
+	if (section.isAddressFixed) {
+		snprintf(addr, sizeof(addr), "%04" PRIx16, section.org);
+	}
+	if (section.isAlignFixed) {
+		snprintf(mask, sizeof(mask), "%" PRIx16, static_cast<uint16_t>(~section.alignMask));
+		snprintf(offset, sizeof(offset), "%" PRIx16, section.alignOfs);
+	}
+
+	if (section.isBankFixed && nbbanks(section.type) != 1) {
+		if (section.isAddressFixed) {
+			where = "at $";
+			where += bank;
+			where += ":";
+			where += addr;
+		} else if (section.isAlignFixed) {
+			where = "in bank $";
+			where += bank;
+			where += " with align mask $";
+			where += mask;
+		} else {
+			where = "in bank $";
+			where += bank;
+		}
+	} else {
+		if (section.isAddressFixed) {
+			where = "at address $";
+			where += addr;
+		} else if (section.isAlignFixed) {
+			where = "with align mask $";
+			where += mask;
+			where += " and offset $";
+			where += offset;
+		} else {
+			where = "anywhere";
+		}
+	}
+
+	return where;
 }
 
 // Places a section in a suitable location, or error out if it fails to.
@@ -222,7 +270,7 @@ static void placeSection(Section &section) {
 
 	// Place section using first-fit decreasing algorithm
 	// https://en.wikipedia.org/wiki/Bin_packing_problem#First-fit_algorithm
-	MemoryLocation location;
+	MemoryLocation location = getStartLocation(section);
 	if (ssize_t spaceIdx = getPlacement(section, location); spaceIdx != -1) {
 		std::deque<FreeSpace> &bankMem =
 		    memory[section.type][location.bank - sectionTypeInfo[section.type].firstBank];
@@ -258,80 +306,53 @@ static void placeSection(Section &section) {
 		return;
 	}
 
-	// Please adjust depending on longest message below
-	char where[64];
-
-	if (section.isBankFixed && nbbanks(section.type) != 1) {
-		if (section.isAddressFixed) {
-			snprintf(
-			    where, sizeof(where), "at $%02" PRIx32 ":%04" PRIx16, section.bank, section.org
-			);
-		} else if (section.isAlignFixed) {
-			snprintf(
-			    where,
-			    sizeof(where),
-			    "in bank $%02" PRIx32 " with align mask $%" PRIx16,
-			    section.bank,
-			    static_cast<uint16_t>(~section.alignMask)
-			);
-		} else {
-			snprintf(where, sizeof(where), "in bank $%02" PRIx32, section.bank);
-		}
-	} else {
-		if (section.isAddressFixed) {
-			snprintf(where, sizeof(where), "at address $%04" PRIx16, section.org);
-		} else if (section.isAlignFixed) {
-			snprintf(
-			    where,
-			    sizeof(where),
-			    "with align mask $%" PRIx16 " and offset $%" PRIx16,
-			    static_cast<uint16_t>(~section.alignMask),
-			    section.alignOfs
-			);
-		} else {
-			strcpy(where, "anywhere");
-		}
-	}
-
-	// If a section failed to go to several places, nothing we can report
 	if (!section.isBankFixed || !section.isAddressFixed) {
+		// If a section failed to go to several places, nothing we can report
 		fatal(
 		    "Unable to place \"%s\" (%s section) %s",
 		    section.name.c_str(),
 		    sectionTypeInfo[section.type].name.c_str(),
-		    where
+		    getSectionDescription(section).c_str()
 		);
-	}
+	} else if (section.org + section.size > endaddr(section.type) + 1) {
 		// If the section just can't fit the bank, report that
-	else if (section.org + section.size > endaddr(section.type) + 1) {
 		fatal(
 		    "Unable to place \"%s\" (%s section) %s: section runs past end of region ($%04x > "
 		    "$%04x)",
 		    section.name.c_str(),
 		    sectionTypeInfo[section.type].name.c_str(),
-		    where,
+		    getSectionDescription(section).c_str(),
 		    section.org + section.size,
 		    endaddr(section.type) + 1
 		);
-	}
+	} else {
 		// Otherwise there is overlap with another section
-	else {
 		fatal(
 		    "Unable to place \"%s\" (%s section) %s: section overlaps with \"%s\"",
 		    section.name.c_str(),
 		    sectionTypeInfo[section.type].name.c_str(),
-		    where,
+		    getSectionDescription(section).c_str(),
 		    out_OverlappingSection(section)->name.c_str()
 		);
 	}
 }
 
+static std::deque<Section *> unassignedSections[1 << 3];
 // clang-format off: vertically align values
 static constexpr uint8_t BANK_CONSTRAINED  = 1 << 2;
 static constexpr uint8_t ORG_CONSTRAINED   = 1 << 1;
 static constexpr uint8_t ALIGN_CONSTRAINED = 1 << 0;
 // clang-format on
-static std::deque<Section *> unassignedSections[1 << 3];
+static char const * const constraintNames[] = {
+    "un",
+    "align-",
+    "org-",
+    nullptr, // align+org (impossible)
+    "bank-",
+    "bank+align-",
+    "bank+org-",
+    nullptr, // bank+align+org (impossible)
+};
 
 // Categorize a section depending on how constrained it is.
 // This is so the most-constrained sections are placed first.
@@ -341,11 +362,10 @@ static void categorizeSection(Section &section) {
 	if (section.isBankFixed) {
 		constraints |= BANK_CONSTRAINED;
 	}
+	// Can't have both!
 	if (section.isAddressFixed) {
 		constraints |= ORG_CONSTRAINED;
-	}
+	} else if (section.isAlignFixed) {
-	// Can't have both!
-	else if (section.isAlignFixed) {
 		constraints |= ALIGN_CONSTRAINED;
 	}
 
@@ -357,68 +377,77 @@ static void categorizeSection(Section &section) {
 		++pos;
 	}
 	sections.insert(pos, &section);
+}
 
-	++nbSectionsToAssign;
+static std::vector<Section *> checkOverlayCompat() {
+	std::vector<Section *> unfixedSections;
+
+	if (!options.overlayFileName) {
+		return unfixedSections;
+	}
+
+	for (uint8_t constraints = std::size(unassignedSections); constraints--;) {
+		if ((constraints & (BANK_CONSTRAINED | ORG_CONSTRAINED))
+		    || unassignedSections[constraints].empty()) {
+			continue;
+		}
+
+		for (Section *section : unassignedSections[constraints]) {
+			unfixedSections.push_back(section);
+
+			if (unfixedSections.size() == 10) {
+				return unfixedSections;
+			}
+		}
+	}
+
+	return unfixedSections;
 }
 
 void assign_AssignSections() {
 	verbosePrint("Beginning assignment...\n");
 
 	// Initialize assignment
-
 	initFreeSpace();
 
 	// Generate linked lists of sections to assign
-	nbSectionsToAssign = 0;
+	static uint64_t nbSectionsToAssign = 0; // `static` so `sect_ForEach` callback can see it
-	sect_ForEach(categorizeSection);
+	sect_ForEach([](Section &section) {
-
+		categorizeSection(section);
-	// Place sections, starting with the most constrained
+		++nbSectionsToAssign;
-
+	});
-	// Specially process fully-constrained sections because of overlaying
-	verbosePrint("Assigning bank+org-constrained...\n");
-	for (Section *section : unassignedSections[BANK_CONSTRAINED | ORG_CONSTRAINED]) {
-		placeSection(*section);
-	}
-
-	// If all sections were fully constrained, we have nothing left to do
-	if (!nbSectionsToAssign) {
-		return;
-	}
 
 	// Overlaying requires only fully-constrained sections
-	verbosePrint("Assigning other sections...\n");
+	if (std::vector<Section *> unfixedSections = checkOverlayCompat(); !unfixedSections.empty()) {
-	if (options.overlayFileName) {
+		size_t nbUnfixedSections = unfixedSections.size();
 		fputs("FATAL: All sections must be fixed when using an overlay file", stderr);
-		uint8_t nbSections = 0;
+		for (size_t i = 0; i < nbUnfixedSections; ++i) {
-		for (int8_t constraints = BANK_CONSTRAINED | ALIGN_CONSTRAINED; constraints >= 0;
+			fprintf(stderr, "%c \"%s\"", i == 0 ? ';' : ',', unfixedSections[i]->name.c_str());
-		     constraints--) {
-			for (Section *section : unassignedSections[constraints]) {
-				fprintf(stderr, "%c \"%s\"", nbSections == 0 ? ';' : ',', section->name.c_str());
-				if (++nbSections == 10) {
-					goto max_out; // Can't `break` out of a nested loop
 		}
+		if (nbSectionsToAssign != nbUnfixedSections) {
+			fprintf(stderr, " and %" PRIu64 " more", nbSectionsToAssign - nbUnfixedSections);
 		}
-		}
+		fprintf(stderr, " %s not\n", nbSectionsToAssign == 1 ? "is" : "are");
-
-max_out:
-		if (nbSectionsToAssign != nbSections) {
-			fprintf(stderr, " and %" PRIu64 " more", nbSectionsToAssign - nbSections);
-		}
-		fprintf(stderr, " %sn't\n", nbSectionsToAssign == 1 ? "is" : "are");
 		exit(1);
 	}
 
-	// Assign all remaining sections by decreasing constraint order
+	// Assign sections in decreasing constraint order
-	for (int8_t constraints = BANK_CONSTRAINED | ALIGN_CONSTRAINED; constraints >= 0;
+	for (uint8_t constraints = std::size(unassignedSections); constraints--;) {
-	     constraints--) {
+		if (char const *constraintName = constraintNames[constraints]; constraintName) {
-		for (Section *section : unassignedSections[constraints]) {
+			verbosePrint("Assigning %sconstrained sections...\n", constraintName);
-			placeSection(*section);
+		} else {
+			assume(unassignedSections[constraints].empty());
 		}
 
-		if (!nbSectionsToAssign) {
+		for (Section *section : unassignedSections[constraints]) {
+			placeSection(*section);
+
+			// If all sections were fully constrained, we have nothing left to do
+			if (!--nbSectionsToAssign) {
 				return;
 			}
 		}
-
+	}
-	unreachable_(); // LCOV_EXCL_LINE
+
+	assume(nbSectionsToAssign == 0);
 }

src/link/object.cpp

@@ -77,7 +77,7 @@ static int64_t readLong(FILE *file) {
 	do { \
 		FILE *tmpFile = file; \
 		std::string &tmpVal = var; \
-		for (int tmpByte = getc(tmpFile); tmpByte != '\0'; tmpByte = getc(tmpFile)) { \
+		for (int tmpByte; (tmpByte = getc(tmpFile)) != '\0';) { \
 			if (tmpByte == EOF) { \
 				fatal(__VA_ARGS__, feof(tmpFile) ? "Unexpected end of file" : strerror(errno)); \
 			} else { \
@@ -582,15 +582,16 @@ void obj_ReadFile(char const *fileName, unsigned int fileID) {
 	for (uint32_t i = 0; i < nbSymbols; i++) {
 		if (std::holds_alternative<Label>(fileSymbols[i].data)) {
 			Label &label = std::get<Label>(fileSymbols[i].data);
-			if (Section *section = label.section; section->modifier != SECTION_NORMAL) {
+			Section *section = label.section;
+			if (section->modifier != SECTION_NORMAL) {
+				// Associate the symbol with the main section, not the "component" one
+				label.section = sect_GetSection(section->name);
+			}
 			if (section->modifier == SECTION_FRAGMENT) {
 				// Add the fragment's offset to the symbol's
 				// (`section->offset` is computed by `sect_AddSection`)
 				label.offset += section->offset;
 			}
-				// Associate the symbol with the main section, not the "component" one
-				label.section = sect_GetSection(section->name);
-			}
 		}
 	}
 }

src/link/output.cpp

@@ -183,14 +183,16 @@ static void
 
 			// Output the section itself
 			fwrite(section->data.data(), 1, section->size, outputFile);
-			if (overlayFile) {
+			offset += section->size;
+
+			if (!overlayFile) {
+				continue;
+			}
 			// Skip bytes even with pipes
 			for (uint16_t i = 0; i < section->size; i++) {
 				getc(overlayFile);
 			}
 		}
-			offset += section->size;
-		}
 	}
 
 	if (!options.disablePadding) {
@@ -277,7 +279,10 @@ static void writeSymName(std::string const &name, FILE *file) {
 		uint32_t state = UTF8_ACCEPT, codepoint;
 		do {
 			decode(&state, &codepoint, *ptr);
-			if (state == UTF8_REJECT) {
+			if (state != UTF8_REJECT) {
+				++ptr;
+				continue;
+			}
 			// This sequence was invalid; emit a U+FFFD, and recover
 			codepoint = 0xFFFD;
 			// Skip continuation bytes
@@ -286,8 +291,6 @@ static void writeSymName(std::string const &name, FILE *file) {
 				++ptr;
 			}
 			break;
-			}
-			++ptr;
 		} while (state != UTF8_ACCEPT);
 		fprintf(file, codepoint <= 0xFFFF ? "\\u%04" PRIx32 : "\\U%08" PRIx32, codepoint);
 	}
@@ -337,7 +340,7 @@ static void writeSymBank(SortedSections const &bankSections, SectionType type, u
 
 	symList.reserve(nbSymbols);
 
-	forEachSortedSection(bankSections, [&](Section const &sect) {
+	forEachSortedSection(bankSections, [&symList](Section const &sect) {
 		for (Symbol const *sym : sect.symbols) {
 			// Don't output symbols that begin with an illegal character
 			if (sym->name.empty() || !startsIdentifier(sym->name[0])) {

src/link/patch.cpp

@@ -426,15 +426,14 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector<Symbol> const &fil
 			}
 
 			if (value == -1) { // PC
-				if (!patch.pcSection) {
+				if (patch.pcSection) {
+					value = patch.pcOffset + patch.pcSection->org;
+				} else {
 					errorAt(patch, "PC has no value outside of a section");
 					value = 0;
 					isError = true;
-				} else {
-					value = patch.pcOffset + patch.pcSection->org;
 				}
-			} else {
+			} else if (Symbol const *symbol = getSymbol(fileSymbols, value); !symbol) {
-				if (Symbol const *symbol = getSymbol(fileSymbols, value); !symbol) {
 				errorAt(patch, "Unknown symbol \"%s\"", fileSymbols[value].name.c_str());
 				sym_DumpLocalAliasedSymbols(fileSymbols[value].name);
 				isError = true;
@@ -444,7 +443,6 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector<Symbol> const &fil
 			} else {
 				value = std::get<int32_t>(symbol->data);
 			}
-			}
 			break;
 		}