Refactor section creation errors, and simplify output for single errors

[Rangi42]

The refactoring here is because the #define sectError macro was an awkward hack, relying on each of its callers to have a variable specifically called nbSectErrors.

The simplification is because most sectErrors are exclusive (note all the if-else chains) so you rarely end up with more than one (note that I had to add a new test for multiple errors with the same section). Comparing these two outputs:

• A non-fatal error that says "Section had this specific flaw", followed by FATAL "Cannot create section "<name>" (1 error)"
• A FATAL "Section "<name>" had this specific flaw"

I find the latter new output easier to read, and less likely for inexperienced users to tunnel-vision read the very last error and miss its context. (Especially when they just post a screenshot/quote of the last error.)

[ISSOtm]

There's a reason I'm not writing C++ anymore...

[ISSOtm]

Please do not use std::function unless you have to. Its indirection has a lot of overhead, including in codegen, due to the type-erasure that it performs.

If you want a callable that can hold onto state (thus, not just a function pointer), please template this function. (Using concepts to document the expected parameters and return type, of course.)

[Rangi42]

It's a fatal error; the overhead of a memory allocation compared to passing a function pointer really won't matter. Regardless, I'd be fine with changing the implementation, if we can do so without moving the whole implementation into the .hpp file as a template (and thereby also having to include the appropriate headers, helper functions, etc).

[ISSOtm]

🦀

[ISSOtm]

I remain concerned about how much extra C++ standard library this pulls in, the extra headers involved, and all the extra codegen regardless of the runtime cost.

[Rangi42]

I'll see if it can be done another way, but I suspect I chose std::function in the first place for errorNoTrace because the callbacks need to capture variables, and the function can't/shouldn't be defined as a template in the asm/warning.hpp header file. If the #include <functional> has enough extra codegen to matter, it should be measurable, not just "avoid std::function all the time".

[Rangi42]

🦀

How would this be solved in Rust? Can format! args be bundled into a Vec of deferred arg packs?

[ISSOtm]

Yes, using format_args!(); but rather, you don't need to provide declaration separately from implementation and thus there are no header woes anymore.

[Rangi42]

If we move fatalNoTrace into the warning.hpp header file, we should do the same for the already-existing errorNoTrace function, which also takes a std::function<void()> callback. However, errorNoTrace calls the static void incrementErrors() function, so then that whole function would have to be non-static to be visible in the header file. Also the header would have to have #include <stdio.h> and #include "style.hpp" for the functions' implementations. I just doing think avoiding std::function is worth that cost.

[Rangi42]

std::function is likely to have a "small buffer optimization", and since sectAlreadyDefinedCallback only captures one reference-pointer, it's likely to fit in a small buffer and not require an extra heap allocation. Unlike a template parameter, it won't be inlined, but that's just the time+space cost of one vtable'd function call. It's not done in some tight loop, it's only done in error conditions, and fatalNoTrace exits immediately so it will only happen at most once. So the runtime+space cost of std::function is low, and the code quality benefit is worthwhile.

[ISSOtm]

I really hate the logic of this function, and would rather use std::format.

[Rangi42]

Can we rely on std::format being available on all platforms/compilers? (Like how std::filesystem isn't quite usable either.) If so, I'd like to replace all our message-formatting code with it, instead of fprintfs.

Even without std::function, I could make this be a template function instead of a stdarg.h one, if you prefer?

[ISSOtm]

Please. As for availability, I have no idea.

[Rangi42]

We could use a template function instead:

template<typename... Args>
void sectError(char const *fmt, Args... args) {
	std::string result;
	int len = snprintf(nullptr, 0, fmt, std::forward<Args>(args)...);
	if (len < 0) {
		fatal("Error describing the error that occurred when merging a section"); // LCOV_EXCL_LINE
	} else if (len > 0) {
		result.resize(len);
		snprintf(result.data(), len + 1, fmt, std::forward<Args>(args)...);
	}
	sectErrors.push_back(result);
}

...however, snprintf then complains "warning: format not a string literal and no format arguments", and I'd rather not disable -Wno-format-security just for this.

Switching to std::format can be done for #1973.