Skip to content

Adds macros for compile time check of variadic format-like calls - #3087

Merged
ivan-mogilko merged 7 commits into
adventuregamestudio:masterfrom
ericoporto:add-compile-time-format-type-check
Sep 17, 2026
Merged

ivan-mogilko merged 7 commits into
adventuregamestudio:masterfrom
ericoporto:add-compile-time-format-type-check

Conversation

@ericoporto

@ericoporto ericoporto commented Sep 13, 2026

Copy link
Copy Markdown
Member

Hey, I am trying to do what I mentioned in a comment

#3084 (comment)

This PR adds three macros that when used in the variadic function calls in ags, allow for type checking at compile time. Unfortunately (expectedly?) adding made me find a gazillion number of issues with the debug/log of stuff (mostly 32-bit / 64-bit builds mismatch). I haven't tested the SAL stuff on MSVC btw, I am mostly using the CI and macOS builds.

Edit: noticed I have no idea what these casts below mean, I fixed format errors in this file that I found in a different way.

Debug::Printf(kDbgMsg_Error, "\tWFN: bad table address: %jd (%jd - %jd)", static_cast<intmax_t>(table_addr),
static_cast<intmax_t>(WFN_FILE_SIG_LENGTH + sizeof(uint16_t)), static_cast<intmax_t>(used_data_size));

@ericoporto
ericoporto force-pushed the add-compile-time-format-type-check branch 2 times, most recently from 159cec3 to 96a29b9 Compare September 13, 2026 23:43
@ericoporto
ericoporto marked this pull request as ready for review September 14, 2026 00:00
@ericoporto

Copy link
Copy Markdown
Member Author

Looks like it finally builds everything now. I am curious if I should leave all individual fixes in many commits or if I should squash everything in a single "All: string format fixes"

Comment thread Engine/debug/debug_log.h Outdated
@ivan-mogilko

ivan-mogilko commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

I have few notes about the macro declarations.

  1. The "Usage" comment is misleading where it compares situations with GCC/Clang and MSVS, because at first it looks like it is suggesting to use different variants in code depending on a compiler. I think these differences do not belong to "usage" explanation but rather to macro implementation. If someone wants to know how to use the macros they do not need to worry about how they are figured out by different compiler. So the "Usage" part should state simply: "in case of a static function use this, in case of a non-static member function use that".

  2. Macro use for non-static members requires extra index increment. That is easy to forget and may become a source of mistakes. What about having a separate macro for this case? Like, something with THIS, or OBJECT in the name, or else (e.g. AGS_FORMAT_STRING_HAS_THIS).

  3. A minor suggestion, I would perhaps reserve a compiler option for disabling these, just in case someone has to make a build without these checks for some reason.

About the formatting fixes, I trust the compiler checks are correct to point about these mistakes. You may squash these commits if you like or leave them, as you prefer. I think critical fixes must be backported to earlier branches, like the one with the missing arguments.

@ericoporto

ericoporto commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

A minor suggestion, I would perhaps reserve a compiler option for disabling these, just in case someone has to make a build without these checks for some reason

ah, ok, SDL does it too and I mostly copied the rest from them

https://github.com/libsdl-org/SDL/blob/dc05826028d6850ccc48661010ac444942cec0af/include/SDL3/SDL_stdinc.h#L1199

I guess I could name it as AGS_DISABLE_FORMAT_MACRO or AGS_DISABLE_FORMAT_CHECK

@ericoporto
ericoporto force-pushed the add-compile-time-format-type-check branch 3 times, most recently from 32e081b to 6c2ea2f Compare September 17, 2026 02:02
@ericoporto
ericoporto force-pushed the add-compile-time-format-type-check branch from 6c2ea2f to 3fe1ffc Compare September 17, 2026 02:31
@ericoporto

Copy link
Copy Markdown
Member Author

hey, I tried to move all the stuff that had to do with 64-bit/32-bit format like differences to a single last big commit, and left the other issues in separate commits before, in case you think anything there is worth porting over to some other branches.

I tried also to address the previous comment, and I added an _OBJECT variant of the macro for using on things that are member of something. Please review, again - I know this is a lot of stuff, but hopefully the new separate commits makes it a bit easier.

@ericoporto
ericoporto force-pushed the add-compile-time-format-type-check branch from 3fe1ffc to aceee1b Compare September 17, 2026 02:38
fix variadic expansion in system debug printf with no args
fixes mostly lld that should be PRId64, zu/size_t and similar for proper 64-bit builds.
@ericoporto
ericoporto force-pushed the add-compile-time-format-type-check branch from aceee1b to ee8221d Compare September 17, 2026 02:40
@ivan-mogilko
ivan-mogilko merged commit af34cd4 into adventuregamestudio:master Sep 17, 2026
26 checks passed
@ericoporto
ericoporto deleted the add-compile-time-format-type-check branch September 17, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants