Frame pointers on: dbghelp's stack walker gets usable frames in crash
reports. /Oy- rather than -fno-omit-frame-pointer -- clang-cl rejects
the GNU spelling:("unknown argument ignored"), and cl.exe takes /Oy-
too. x86-only option; a 64-bit target would ignore it and rely on
unwind data instead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OBJECTTYPE::exists() and SOLDIERTYPE::exists()/DeleteSoldier() are
called on null pointers by design and guard with `this == NULL`. That
is undefined behaviour, so clang infers `this` is non-null: it deletes
the guard inside the callee *and* deletes null checks that follow a
call in the caller, which is an access violation at /O2 in code MSVC
has always compiled the naive way. The caller-side inference happens
in every translation unit that calls one of these, so this has to be
global rather than per-file; there is no per-function attribute or
pragma for it. Drop it once nothing relies on a null `this`.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The client would send up to 256 KB and the sink answered a settling 400 above
64 KB, so the two disagreed about what a valid report is, and the client
deletes what it is told is invalid. Unreachable in practice — the module table
and backtrace are both bounded, which puts the ceiling near 10 KB — but the
two constants have to agree for the disagreement not to matter.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The endpoint is public and unauthenticated, so the whole uploaded file is
attacker-chosen, not only the part the client copied from Ja2.ini. The build
field sat inside backticks a backtick closes, and the access-violation text
went in raw, so either could carry markdown or a link into the channel.
One clean(): printable ASCII minus what Discord reads as markup or a URL,
length-capped. It replaces the handle's own stripping and absorbs the .trim()
the field getters did, which also drops the CR that "(.+)$" captures off a
CRLF report.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The module table is what makes a report symbolizable, but a full path also
carries the player's Windows account name and wherever they keep the game,
and the report leaves their machine. Symbolizing only ever matched on module
name and base — symbolize_crash already ran every path through baseName()
before printing it — so the directory was read by nothing.
Say so in the consent prompt too. "No personal information" was not true of
a table full of paths, and is only worth claiming if it holds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds tools/symbolize_crash.cpp, built alongside the game as a console exe:
it reads a crash_report_*.txt written by writeExceptionBacktrace and prints
the backtrace with function names, source lines and inlined frames, in call
order, coloured when standard output is a console.
Symbols come from DbgHelp against the build's PDB rather than an external
symbolizer. SymLoadModuleEx takes the runtime base out of the report's
module table, so the relocation arithmetic /DYNAMICBASE forces on us is
DbgHelp's problem now; a report predating the module table loads at the
image's preferred base, which is where it ran. Verified under Wine: PDB
line info and inline traces both resolve.
The tool wants C++23 (std::print, std::format) where the game is C++17, so
the standard is set on the target alone. Both MSVC 19.51 and clang-cl build
it clean in all three configurations.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The other half of the feature, and the half that decides whether a player's report
survives: the endpoint CRASH_TELEMETRY_URL points at. It takes the POST from
processCrashTelemetry and forwards the report to a Discord webhook as a file
attachment. It stores nothing -- a report is only worth reading next to the PDB it
was built against, and that never leaves a developer's machine, so there is nothing
for a bucket to do here that the channel we already read bug reports in does not do
better. That also keeps the whole thing inside the Workers free tier, where the
10 ms budget is CPU, not wall clock, and waiting on Discord is not metered.
In the repo rather than in someone's home directory because the status codes are a
contract with the client and the two have to be changed together. reportIsSettled()
in sgp/crash_telemetry.cpp deletes the player's copy on 2xx and on 400/413/415 and
keeps it on everything else, so a settling 4xx returned for a failure on our side
silently destroys the report. Every failure path here is therefore a 503, each
naming its own cause in wrangler's console, and the one 4xx that is safe -- 429,
which the client does not settle -- is the one the rate limiter returns.
That rate limiter is a binding with a .limit() call, not a dashboard rule: WAF rate
limiting rules need a zone and a workers.dev subdomain is not one. Per-IP, 50 a
minute, which has to clear kMaxUploadsPerRun (20) in the client or a player draining
a backlog throttles themselves. It is checked before the body is read.
A report is attacker-controlled text arriving at a public, unauthenticated endpoint
whose URL ships in every player's Ja2.ini, so the summary line strips markdown from
the player handle and the payload sets allowed_mentions to nothing. The size cap and
the "*** CRASH" check keep drive-by scanners out; anything determined gets through,
and the blast radius is a message we delete.
test.mjs covers the whole contract against a stubbed fetch, no network and no
webhook needed. DISCORD_WEBHOOK is a secret and lives nowhere in this tree;
.dev.vars, which holds a live one for local development, is gitignored.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A crash report is worth nothing sitting on the player's disk. On startup, drain
the crash_report_*.txt files the handler left behind to the endpoint named by
CRASH_TELEMETRY_URL in Ja2 Settings; an empty or absent key turns the feature off
entirely. The first launch asks the player once and remembers the answer in
telemetry.consent -- declined means the reports simply keep accumulating locally.
This lands in its own translation unit rather than in more of debug_win_util.cpp.
Everything in that file runs inside a faulting thread and may not allocate;
everything here runs at startup with a healthy heap and is ordinary code. Two
files keep the no-heap rule easy to see and easy to hold.
The draining runs on a detached thread. The uploads are synchronous WinHttp calls
with seconds-long timeouts, and this sits on the startup path, so on the main
thread an unreachable endpoint is a stall the player watches before the splash
screen. Nothing waits on the result: if the player quits first the process exits
from under the thread, which costs nothing, since an interrupted upload leaves the
file on disk and it goes out next launch. The consent prompt stays on the main
thread on purpose -- it is a question, and a question has to be asked before
anything is sent.
Which reports get deleted is chosen so that a mistake cannot destroy them. A file
goes away on 2xx, and on 400/413/415, i.e. content the server will never accept.
Everything else keeps it: no connection, 5xx, and notably the 403/404 of a
mistyped CRASH_TELEMETRY_URL, which would otherwise silently eat every player's
crash history. Bounds all round: every WinHttp phase has a timeout, a report over
256 KB is not one of ours and never goes on the wire, at most 20 uploads per
launch so a crash-looping build cannot turn startup into an upload session, and
reports older than 30 days are dropped unsent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Report crashes for every player, not only those running under Wine's WINEDEBUG.
A last-chance UnhandledExceptionFilter is no good: faults on the message-pump /
WindowProcedure path have no game __except on their stack, and under Wine the
WndProc dispatch swallows them before they ever reach "unhandled". Install a
vectored handler instead -- it runs first-chance, ahead of every frame handler,
records the fault and returns EXCEPTION_CONTINUE_SEARCH, so normal SEH is
unaffected.
The dump is deliberately heap-free. The crash most worth reporting is often heap
corruption, so anything that allocates (the game Logger, std::vector, DbgHelp's
Sym* family) would fault again and take the report down with it. Using only stack
buffers and raw Win32, writeExceptionBacktrace writes the registers, the faulting
address, a UTC timestamp, the build id, the loaded-module table and a
bounds-checked manual EBP walk into a fresh numbered crash_report_NNN.txt.
The module table is what makes the addresses mean anything. Our executables are
linked /DYNAMICBASE and keep their .reloc section, so the loader is free to move
the image: Wine leaves it at the preferred base, Windows ASLR does not. A report
listing only runtime addresses is symbolizable by luck, and silently wrong once
the luck runs out. Recording where each image actually landed turns the offset
into arithmetic:
llvm-symbolizer --obj=JA2.exe --adjust-vma=$((<JA2.exe base> - 0x400000))
It is walked off the PEB loader list because both alternatives -- EnumProcessModules
and DbgHelp's module APIs -- allocate, and this path must not. Having the table
also frees the backtrace from restricting itself to our own module's return
addresses: a fault inside ddraw/fmod/bink is exactly the case worth seeing, and
an address belonging to no module is recognizable as the frame-pointer debris it
is.
Two things go into the report beside the machine state. czVersionString stamps
the build, so a report matches the exact PDB it has to be symbolized against, and
the optional HANDLE from Ja2 Settings names the player, so a report can be tied
to whoever raises it with us. The handle is sanitized where it is set rather than
where it is used: it is player input that lands in a line-oriented text report,
so anything that could forge a line (CR/LF, control and non-ASCII characters) is
dropped and the length is capped.
Finally the player is told. The message is composed in the handler, while the
fault details and the report's filename are still in hand, but shown from
SGPExit: first-chance means the exception may yet be handled downstream, and a
message box pumps messages, which on the heap that just faulted is a second
crash. It replaces the generic "Unhandled exception. Unable to recover." box
rather than adding to it -- it says more, and it names the file we need attached
to the bug report.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GameVersion.cpp becomes a configure_file template, so the version strings are a
build input rather than something a CI step seds into the working tree mid-run.
That is what lets a crash report be matched to its exact build's PDB for offline
symbolization.
The two strings get distinct jobs. czVersionString is the machine-readable build
identity: the bare short commit SHA and nothing else. A `git describe` string
cannot serve here -- czVersionString is 16 bytes because it is stored in the
savegame header as SAVED_GAME_HEADER::zGameVersionNumber and strcmp'd on load,
and describe puts the SHA last, so truncation ate exactly the discriminating part
and could make two different commits compare equal. CI passes 9 hex; anything
that would not fit is now a configure error rather than a silent clip.
zBuildInformation is the display string, untruncated, and it is no longer printed
beside czVersionString: that drops the duplication in the version line ("JA2 1.13
V4-555-G6A941C0 2026-07-25 V4-555-G6A941C06") and stops showing a token that is
now machine-only. CI's copy leads with GAME_VERSION, because the version line is
now the only place a player reads which release they are running.
Finish what the sed step's TODO asked for: the workflow pins GAME_BUILD_INFORMATION
and GIT_SHA once, next to the gamedir SHAs it already pins, and hands them to CMake
with -D. Every parallel build job therefore stamps one agreed identity, and the
compile checkout needs no git history at all.
A build nobody identifies is a local build and says so -- czVersionString reads
"local" -- rather than deriving a SHA from HEAD. A derived SHA could not stay
true: nothing re-runs configure once HEAD moves, so an incremental build would
keep whatever it was configured with, and keeping it honest would mean
reconfiguring on every commit. It would also buy nothing, because only packaged
builds are archived with their PDBs; a local build's crash reports are symbolized
against the PDB sitting next to the exe.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gubFOVDebugInfoInfo is a heap-allocated UINT8*, so sizeof() on it only
covered the pointer, not the WORLD_MAX-sized buffer it points to.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
binkw32.lib as shipped is a long-format ordinal import library built in
2002, and link.exe is the only linker left that binds it. lld-link takes
it without complaint and writes an import descriptor with an empty thunk
table, so every Bink call reaches whatever the stale IAT slot holds. The
first one is BinkSoundUseDirectSound, reached from BinkInitialize during
EnterIntroScreen, which is unconditional -- Intro.cpp constructs its
VideoPlayer with VT_SMK|VT_BINK in every application, so a Smacker-only
gamedir still runs the Bink init path. The process dies before the splash
with an access violation executing an unmapped address.
clang-cl.cmake and mingw.cmake already rebuilt the library from
binkw32.def for exactly this reason, but they are toolchain files, so the
rebuild only happened when someone cross-compiled. Which linker is in use
is not a cross-compilation question: a preset that points
CMAKE_CXX_COMPILER at clang-cl -- how you use it from Visual Studio --
loads no toolchain file and linked the broken library instead. The test
was a proxy for "is this lld-link", correct until it wasn't.
So the rebuild moves to the top-level CMakeLists and runs unconditionally,
and clang-cl.cmake loses its copy: it already sets CMAKE_AR to llvm-lib,
which is the same binary the new code invokes with the same arguments.
mingw.cmake keeps its own, because GNU ar cannot build an import library
at all and dlltool takes different flags.
Only the name shape differs between archivers. lib.exe prepends the x86
leading underscore to every name in the .def; llvm-lib and llvm-dlltool
take the name as written. The checked-in file keeps the underscore for the
llvm tools and the MSVC path strips it back off.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No warning is fixed here. Every one we currently emit is harvested and
switched off one by one, and /WX goes on top, so both toolchains build
quiet and any *new* warning fails the build instead of scrolling past.
cmake/Warnings.cmake carries two independent lists, one per compiler,
one line per warning with its occurrence count. Deleting a single line
is the unit of work from here on: drop the line, fix the fallout in
both compilers, commit. The build stays honest in the meantime.
Picking a level had to come first. CMake stopped injecting /W3 of its
own accord (policy CMP0092), which had quietly left MSVC on the /W1
default and hid nearly everything; at /W4 it reports 89361 warnings
across 31 codes. clang-cl is pinned to /W3 -- clang's own -Wall, and
already broader than MSVC /W4 -- for 55 more. The two lists are not
comparable by length.
Counts cover all four applications, since the #ifdef forest means each
one compiles different code. The file is included after the ext/
subdirectories so that add_compile_options' directory scope leaves
vendored code on its default flags; ext/ still warns, deliberately,
and /WX does not reach it.
The existing /wd4838 moves here with the rest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
every build artifact also carries Ja2Export.exe, so the glob handed mv
three paths and it failed with "target 'gamedir/ja2.exe' is not a
directory". The assemble job runs on Windows, so the lowercase matrix
name matches the uppercase target file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A skipped job still shows up in a pull request's check list, so assemble
and release reported there as two permanently skipped checks. A job can
only be left out of a run graph it does not belong to, so move both into
release.yml, which triggers on master, v* tags and manual dispatch, and
gets its build by calling build.yml through workflow_call. A pull request
now runs build.yml directly and reports exactly the five checks it can
actually pass.
The global_vars step only existed to compute assemble_release for the two
jobs that left, and it read the workflow_dispatch input, which a called
workflow cannot see. Its logic moves to a condition on assemble itself,
unchanged: a manual run assembles only when asked, tags always assemble,
master assembles only upstream. The release job needs no condition since
a skipped assemble skips it.
Check out the source rather than cloning it by hand. A pull request
builds its merge commit, which is on no branch and so is absent from a
clone of branches and tags; it resolved only because --filter=tree:0 made
the clone partial and git fetched it from the promisor remote on demand.
The action fetches the merge ref itself, so nothing hinges on the filter.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A push builds in the repository it was pushed to, so a contributor's
branch builds in their fork and the base repository sees nothing. Without
a run of its own there is no check on the pull request for branch
protection to require, so trigger on pull_request as well.
Push builds narrow to master and v* tags: with pull requests covered,
building every branch push only duplicated the same commit's build across
four Windows matrix jobs. Master still builds so the "latest" pre-release
keeps updating. A branch with no pull request open builds via
workflow_dispatch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
'typename' disambiguates a qualified dependent name, telling the
compiler that something like T::iterator names a type rather than a
value. Before a plain identifier there is nothing to disambiguate: P1
is already a type, and so is PopupIndex.
The grammar wants a qualified name after the keyword, so clang reports
"expected a qualified name after 'typename'", drops the keyword and
carries on -- 15175 times, since the offending declarations sit in
widely included headers. MSVC accepts them without a word.
Semantics do not change. Both compilers already ignored the keyword,
so the partial specialisations matched before this and match after it.
The diagnostic belongs to no -W group and therefore cannot be switched
off with -Wno-, which makes deleting the tokens the only way to quiet
it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Compiler cache stored in the GitHub Actions cache, shared across runs.
Requires /Z7 instead of the CMake default /Zi, since sccache cannot cache
objects whose debug information lands in a shared PDB. That is a debug
information format change only, the generated code is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The tree compiles and links under clang-cl, so make that a configuration
anyone can ask for rather than a command line to reconstruct:
MSVC_DIRECTORY=/path/to/msvc cmake --preset '1dot13 Release clang-cl'
It is a check on the source, not a port -- MSVC still builds what ships. What
it buys is a second front end over every translation unit, which is how the
inline assembly with no operand size and the resource string with a raw 0xA9
were found, and it runs natively: clang-cl, lld-link, llvm-rc and llvm-lib
need no Wine, so a full JA2.exe takes a fraction of the time.
The toolchain file names every MSVC and SDK include directory, because
clang-cl does not read INCLUDE and LIB the way cl does, and picks the newest
version of each tree rather than pinning one. MSVC_DIRECTORY says where they
live and is reported plainly when it is missing or wrong.
build a bindable Bink import library when not building with MSVC
The shipped binkw32.lib is an old ordinal-based long-format import library.
MSVC's link.exe binds it, but clang-cl's lld-link does not: it leaves every
Bink import unresolved, so at run time the calls jump into arbitrary code and
crash the game at startup, six access violations deep in unrelated code before
the game's own handler gives up.
Rebuild the import library from binkw32.def with llvm-dlltool on any front end
other than MSVC. The library imports by ordinal, for the reasons binkw32.def
records: binkw32.dll exports decorated stdcall names that keep their leading
underscore (_BinkOpen@8), and llvm-dlltool strips that underscore from the
imported name whatever --no-leading-underscore says, so a by-name import would
look up a name the DLL does not export and Wine would substitute a builtin stub
that aborts ("unimplemented function binkw32.dll.BinkSetSoundSystem@8").
Ordinals have no such ambiguity and match the retail DLL. MSVC keeps binding the
shipped library, so the retail build is untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
refactor toolchain
make the preset template something CMake will read
The template carries two // comments. JSON has no comments and neither does
the preset format, so CMake refuses the whole file:
CMakePresets.json:10: Missing '}' or object member name
Which means the file CopyUserPresetTemplate drops into the source directory
has never worked as handed over -- "cmake --preset '1dot13 Release'" fails
until you delete the comments yourself.
The text they carried is worth keeping, so it moves to the description field
the format does have, and cmake --preset '1dot13 Release' now configures.
Two link settings were conditional on the Visual Studio generator, though
what they compensate for is the prebuilt libraries in the tree, which are the
same libraries under every generator. Ninja builds got away with it because
link.exe is quiet about both; lld-link is not.
legacy_stdio_definitions.lib: RakNetLibStatic.lib still calls _vsnprintf,
which the UCRT only exports through that compatibility library.
/SAFESEH:NO: lua51.lib and the smackw32 import library carry no safe
exception handler table, so no linker can emit one. The image never had it.
Before and after, JA2.exe reports SEHandlerTable 0x0 and SEHandlerCount 0,
with DYNAMIC_BASE, NX_COMPAT and TERMINAL_SERVER_AWARE unchanged -- the flag
states what link.exe was already doing silently.
Dbghelp.lib and Winmm.lib are also spelled as the SDK ships them, since a
cross-compile looks for them on a case sensitive filesystem.
editscreen.cpp and LoadScreen.cpp each end in an "#else //non-editor
version" branch defining EditScreenInit/Handle/Shutdown and the LoadSaveScreen
trio. jascreens.cpp defines the same six under "#ifndef JA2EDITOR", so a
non-editor build compiles both.
Nothing linked the editor copies. The map file for JA2.exe resolves all six
to JA2_Ja2:jascreens.cpp.obj, link.exe having taken the first archive that
answered and never pulled the other. lld-link pulls both and calls it what it
is: six duplicate symbols.
The two sets are not identical -- the editor Handle stubs return ERROR_SCREEN
where jascreens returns TRUE -- so this removes the copies that lose, leaving
the behaviour the linked image already had.
The one byte in ja2.rc above ASCII is the 0xA9 in "Copyright (c) 1998",
written raw. What it means depends on the codepage the resource compiler
happens to run under, and llvm-rc will not guess: "Non-ASCII 8-bit codepoint
can't be interpreted in the current codepage".
An escape in a wide string says it outright. The escape alone is not enough
-- a byte above 127 cannot appear in a narrow resource string either -- but
the value block is stored as wide characters regardless, so the L costs
nothing.
rc.exe produces a byte-identical ja2.rc.res before and after.
The pattern compare in Blt16BPPBufferPixelateRectWithColor gives neither
operand a size: a memory reference with no register to take the width from,
against an untyped zero. MSVC picks a byte, which is right -- Pattern is
UINT8[8][8], and every other access to it in the block goes through al --
but the source never says so, and clang-cl refuses to guess.
Naming the size changes nothing. cl emits 80 3C 1E 00 before and after.
Four string literals carry the bytes A3 A5, which is GBK for U+FF05, the
fullwidth percent sign. All four sit in g_lang == i18n::Lang::zh branches
added by "Chinese specific update (by zww from tbs)", and each has an ASCII
sibling in the else branch using an ordinary percent.
clang-cl decodes source as UTF-8, where A3 is not a valid lead byte:
error: illegal character encoding in string literal
A raw high byte in a literal means whatever the compiler's source charset
says it means, so this text has never been portable. Asking the cl we build
with what it makes of L"%d%<A3><A5>%%" gives a 7-character string whose
fourth character is 163 -- it reads the bytes as Latin-1 and produces two
characters, £ and ¥, not the one the author wrote. The same probe on
L"%d%％%%" gives 6 characters with U+FF05 in that position.
So the Chinese text has been rendering as garbage in any build not made on a
CP936 machine, this one included. Writing the character as ％ says
exactly which character is meant regardless of the compiler's charset, and
restores what the branch was written to display.
The format grammar is untouched. In each of these strings the character is
preceded by a percent, making %<fullwidth percent> an unknown conversion
specification that prints the character following it -- the same trick the
else branches spell as %%. Where a string is formatted twice, once into a
buffer and again as the format argument of mprintf, the doubling in the
sibling literal (%%%%) is what survives two passes; that pairing is
unchanged.
Both files are now pure ASCII:
python3 -c "print(sum(1 for b in open(F,'rb').read() if b>0x7f))"
The parse sweep drops from 5 error sites in 3 files to 1 in 1 -- the inline
assembly in sgp/vobject_blitters.cpp is all that is left.
DrawTextToScreen takes STR16, a mutable wchar_t*, for a string it only
measures and prints. Quest Debug System.cpp picks its text with a conditional:
DrawTextToScreen( gubFact[ usLoop1 ] ? L"True" : L"False", ... );
which has type const wchar_t*. clang-cl's MSVC compatibility lets a bare
string literal decay to wchar_t*, but not a const wchar_t* expression, so it
reports
error: no matching function for call to 'DrawTextToScreen'
note: candidate function not viable: 1st argument ('const wchar_t *')
would lose const qualifier
The parameter is const-correct in every function DrawTextToScreen hands the
string on to, so the change follows the string down the call chain rather
than stopping at the top:
DrawTextToScreen, ShadowText Utils/WordWrap.h
WFStringPixLength Utils/Font Control.h
mprintf, StringPixLength, sgp/Font.h
VarFindFontCenterCoordinates,
VarFindFontRightCoordinates
WinFontStringPixLength sgp/WinFont.h
gprintfdirty TileEngine/Render Dirty.h
The varargs members of that list copy the format string into a local buffer
before doing anything with it, and the measuring ones walk it and return a
width; none of the nine assigns through the parameter. Where StringPixLength
casts the string to UINT16* to walk it, the cast and the local now carry the
const rather than dropping it.
Callers are unaffected: every one already passes something that converts to
const CHAR16*. Both configurations build with no new diagnostics, and the
parse sweep drops from 9 error sites in 7 files to 5 in 3.
To check the claim that nothing writes through these parameters, read the
nine bodies; each is short.
std::uniform_int_distribution::operator() takes its engine by non-const
reference, and the call passes a freshly constructed std::mt19937 temporary.
MSVC binds it anyway; clang-cl reports
error: no matching function for call to object of type
'std::uniform_int_distribution<>'
note: candidate function not viable: expects an lvalue for 1st argument
Naming the engine is enough. Behaviour is unchanged: the engine is still
seeded from std::random_device and still discarded after the single draw.
TestTableTemplate declares SetRefresh and Init but never defines them; both
exist only as explicit specialisations, one set per table, at the bottom of
FacilityProduction.cpp and MilitiaWebsite.cpp.
That is too late for tables 3 and 4. Both files call SetRefresh on their
table hundreds of lines above the specialisation, and a call is a point of
instantiation, so the compiler has already committed to instantiating the
primary template by the time the specialisation shows up. A specialisation
declared after the first use that would instantiate it is ill-formed; MSVC
takes it anyway, clang-cl reports
error: explicit specialization of 'SetRefresh' after instantiation
Declaring the specialisations after the includes fixes it. Tables 1 and 2 are
only ever reached through the base class pointer, so they were not diagnosed,
but they are declared alongside the others rather than left to depend on that.
The definitions stay where they are.
FileWrite took its source as PTR, a mutable void*, though it only reads it:
the buffer goes straight to vfs::tWritableFile::write, which takes a const
vfs::Byte*. STIConvert.cpp pads a file a byte at a time with
FileWrite(hFile, "", 1, NULL);
and a string literal is a const char[1], which PTR cannot hold.
Declaring the parameter const void* matches what the function does and what
the layer below it already asks for. The cast inside becomes const as well.
None of the 431 calls change: every pointer, const or not, converts to
const void*.
JA2EncryptedFileWrite and NewJA2EncryptedFileWrite have the same shape but
transform the buffer on the way out, so they keep PTR.
Verification:
ninja -C build parse # STIConvert.cpp clean
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The parameter was STR8, a mutable char*, for a filename the function only
reads: it runs strstr over it and hands it to SoundPlay. environment.cpp picks
between two of them at runtime,
PlayJA2SampleFromFile( Chance( 50 ) ? "Sounds\\WATERSNAKE_ATTACK_01.WAV"
: "Sounds\\WATERSNAKE_ATTACK_02.WAV", ... );
and the conditional is a const char*, which STR8 has no room for. Literals
passed on their own are still accepted under the MSVC rules, which is why only
these two calls ever failed.
The cast at the call to SoundPlay was already written as (STR) and is left as
it is. SoundPlay, SoundPlayStreamed and SoundLoadSample below it all still
take STR, so making them const is a separate piece of work and a much larger
one; until then that cast is where const stops.
No call site changes. The (char*) cast at LuaInitNPCs.cpp:11640 is now
unnecessary but still valid, and is left for whoever next edits that line.
Verification:
ninja -C build parse # both environment.cpp sites gone
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
STR16 is CHAR16*, so ScreenMsg asked for a mutable format string it has no
intention of mutating: the body only hands it to va_start and vswprintf, and
vswprintf takes it as const.
Calls that pass a literal are accepted anyway under the MSVC rules clang-cl
applies. Calls that pass anything else are not, and DynamicDialogue.cpp picks
the format at runtime:
ScreenMsg( FONT_MCOLOR_LTGREEN, MSG_INTERFACE,
...sOpinionModifier >= 0 ? L"%s: %s +%d" : L"%s: %s %d", ... );
The conditional has type const wchar_t*, and there is nowhere for it to go.
Declaring the parameter const CHAR16* is what the function always meant. Every
existing caller still compiles: a literal and a CHAR16* both convert.
Verification:
ninja -C build parse # DynamicDialogue.cpp clean
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
char team_names[][30];//hayden need client_names with AI
This is not a declaration of anything. C would treat it as a tentative
definition and settle on one element; C++ requires a size or an initializer,
and clang says so. MSVC accepts it quietly.
Nothing in the tree reads or writes team_names, and the comment reads as a
note to self rather than a feature. Rather than guess a size for an array
nobody uses, it goes.
Verification:
grep -rn 'team_names' --include=*.cpp --include=*.h . # nothing
ninja -C build parse # client.cpp clean
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The AI deadlock message hands ScreenMsg an object where its format string
promises a pointer:
ScreenMsg(..., L"Aborting AI deadlock for [%d] %s %s data %d",
pSoldier->ubID.i, pSoldier->GetName(),
utf8_to_wstring(std::string(szAction[...])), ...);
utf8_to_wstring returns std::wstring by value. ScreenMsg is variadic, so the
whole object is pushed and %s then reads the first bytes of it as a pointer.
What that prints, or whether it survives printing it, is not defined. clang
says as much: the call will abort at runtime.
Adding .c_str() passes what the format asked for. The temporary lives until
the end of the statement, so the pointer is good for the duration of the call.
The only other use of utf8_to_wstring in this file has the same shape and is
commented out; it is left as it is.
Verification:
ninja -C build parse # AIMain.cpp clean
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LoadEquipmentTemplate parsed each line of a saved equipment template like
this:
if ( iss >> num && iss >> (UINT16)node.item )
node.item is already a UINT16, so the cast does nothing except turn the member
into a temporary. The stream then extracts into that temporary, which is
thrown away at the semicolon, and node.item keeps the 0 its constructor gave
it. node.ammoitem, cast the same way on the next line, is lost with it.
This is not only a parse error. The tokens are still consumed, so the line
parses and the node is appended, but every node comes back with item 0. The
consumer skips those:
if ( node.slot >= 0 && node.slot < NUM_INV_SLOTS && node.item != NOTHING )
so loading an equipment template has been quietly doing nothing at all. Only
the attachments, read through a real variable, ever survived, and they were
attached to a node that got skipped.
Removing the two casts makes the extraction write where it was always meant
to. slot is still read through num, as it must be, since it is an INT8 and the
stream would take it for a character.
This changes behaviour: gear templates will start applying. That is the point,
but it is worth knowing before this ships.
Verification:
ninja -C build parse # both Handle Items.cpp sites gone
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
not is an alternative spelling of ! in ISO C++, so it cannot name a variable:
XML_Char const* not = GetAttribute("not", atts);
MSVC only treats it as an operator when iso646.h is included, so it accepts
this as an identifier and the file compiles. clang cannot parse any of the
three lines that mention it.
Renamed to notOp, matching cmpOp thirty lines above, which reads the "op"
attribute the same way. The attribute name in the XML is untouched.
Verification:
grep -rnE '\*[ \t]*(and|or|not|xor)\b' --include=*.cpp --include=*.h . # nothing
ninja -C build parse # parser-error class gone: 21 -> 17 sites
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AutoCArray takes its allocator and deallocator as template parameters:
template<typename T, typename void*(*Alloc)(size_t) = malloc,
typename void(*Dealloc)(void*) = free>
Alloc and Dealloc are function pointers, so they are values, not types, and
typename has no business in front of them. MSVC ignores the keyword there;
clang stops at it and cannot parse the parameter list, which fails every
translation unit that reaches this header.
Removing the two stray keywords leaves the parameters exactly as they were
meant to be read, with the same defaults of malloc and free. Nothing in the
tree instantiates AutoCArray, so nothing depends on the spelling either way.
Verification:
grep -rn 'AutoCArray' --include=*.cpp --include=*.h . # only this header
ninja -C build parse # sgp_auto_memory.h clean
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each of these takes the address of an object that has no name:
bool b = client->Startup(1,30,&SocketDescriptor(), 1);
OutputToStream(msg, &(SGP_LOG(s_log.id)));
MSVC allows it as an extension. The temporary lives to the end of the full
expression, which is past the call, so all three happen to work; clang rejects
the address-of outright.
Naming the object changes nothing about when it is built or what is passed,
only that the pointer now refers to something that outlives the statement.
MemMan.cpp already does this with the same logger call:
sgp::Logger::LogInstance memLeak = SGP_LOG(log_id);
Verification:
ninja -C build parse # address-of-temporary class gone: 24 -> 21 sites
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each of these says again, outside the class, what the class already declares:
template <int N>
void TestTableTemplate<N>::SetRefresh();
SetRefresh and Init are declared as members a few dozen lines above, so these
add nothing. A member named out of line has to be a definition; MSVC lets the
declaration through, clang does not.
None of the three templates ever defines these members generically. Every
instantiation supplies its own, as an explicit specialization next to the
screen that uses it, and those are untouched:
template<> void DropDownTemplate<DROPDOWNNR_APPEARANCE>::SetRefresh()
{ gfIMPPrejudice_Redraw = TRUE; }
The mpSelf lines that follow each of these look similar but are definitions of
a static data member, so they stay.
Verification:
grep -rn 'Template<N>::' --include=*.h Laptop/ # only mpSelf definitions
ninja -C build parse # out-of-line class gone: 28 -> 24 sites
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
