Common, Engine, Tools: error refactor to variadic constructor

[ericoporto]

fix #3028

I am leaving as a draft because I still need to look into it with more care, I know two places there was a format argument but not % pattern to get the argument, which got fixed just because I had to look into it. There is also a few places in mfl_utils.cpp where I got type warnings and the fix is lazy.

The actual error.h refactor was AI authored with minor hand touches. The actual replacement of each calls used a mix of find and replace, hand authoring and AI. I also did this on my Mac, so better let the CI catch any possible issues in other systems. Leaving it as a draft for now.

Gemini wrote that new error_test to when I was trying to reduce/simplify the modified header, because I thought it was too big. It can totally be deleted if we think it is useless.

🚨NOTE: These code changes were authored by AI, I used Gemini Flash 3.7, Claude Code and Codex GPT Sol 5.6🚨

[ivan-mogilko]

I know two places there was a format argument but not % pattern to get the argument, which got fixed just because I had to look into it

Are you speaking of original code, or the new code? If this mistake was in original code, then this fix should be backported to release-3.6.3 and possibly release-3.6.2 branches.

[ericoporto]

Are you speaking of original code, or the new code? If this mistake was in original code, then this fix should be backported to release-3.6.3 and possibly release-3.6.2 branches

It is the two places below, but looking again I am wrong, the second parameter is the “comment” parameter that this PR is removing.

ags/Engine/plugin/agsplugin.cpp

Line 1096 in a323b3e

return new Error("Not a valid AGS plugin: no engine startup entry point 'AGS_EngineStartup' exported.", apl->filename.GetCStr());

ags/Tools/agspak/commands.cpp

Line 37 in a323b3e

return new Error("Failed to parse pack file.", MFLUtil::GetMFLErrorText(mfl_err).GetCStr());

btw, I am not so sure now looking at the changes (talking about the entire PR now) if this is a good idea, since these variadic format stuff are easy to make a mistake and before because the string had to be made through String Format it was obvious what was the format and what was extra stuff.

I think in the end I prefer how it worked before.

[ivan-mogilko]

btw, I am not so sure now looking at the changes (talking about the entire PR now) if this is a good idea, since these variadic format stuff are easy to make a mistake and before because the string had to be made through String Format it was obvious what was the format and what was extra stuff.

Is not it the same situation as with any function that has a string formatting, similar to e.g. logging (Debug::Printf)?

Majority of new Error constructors in the code require string formatting, meaning one has to be typing new Error(String::FromFormat(... every time.

With the removal of useless "comment" argument, the constructor's arguments is only formatted string, and optionally the previous error as a first argument.

[ivan-mogilko]

I noticed there was a comment about adding __attribute__(format) for printf-like functions in the engine (for compilers that support that), but this comment is now removed. In any case, i think that was a good idea.

EDIT: by the way, MSVC also supports a similar thing, although it's more complicated. One has to enable SAL in the build configuration:
https://learn.microsoft.com/en-us/cpp/code-quality/understanding-sal?view=msvc-170
Formatted string annotation:
https://learn.microsoft.com/en-us/cpp/code-quality/annotating-function-parameters-and-return-values?view=msvc-170#format-string-parameters

[ericoporto]

Thanks, I wanted to read more on that before and testing but didn’t found the time to sit in the computer to do so yet.

[ericoporto]

@ivan-mogilko I think I am ok now with these code changes. On the code format check idea, I found some promising code in SDL:

https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_stdinc.h

Look that they include this SAL approach you mention but also some easier way to apply to gcc and the like, below is an example of how they use

https://github.com/libsdl-org/SDL/blob/26b37f5d1c3518125ff8b269b5bdfcf452870500/include/SDL3/SDL_error.h#L89

I imagine a simplified version of these could be made for our usecase.

[ericoporto]

I think I want to have #3087 done before, later this here is easier to rebase on top of that, and readjust the code changes here that are mostly very mechanical.