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>
strstr returns a pointer into the string or NULL, and these four tests compare
that pointer to 0 with >. Ordering a pointer against a null pointer constant
is not something C++ defines; MSVC allows it, clang rejects it.
The intent is plainly "did this attribute name contain the tag", and the
result is the same either way, since the only pointer these can produce that
is not greater than zero is the null one. != NULL says it directly, and reads
like the strcmp(name, "...") == 0 tests it sits between.
strstr rather than strcmp is deliberate here: the attributes it looks for are
numbered, usAttachment1 through usAttachment4 and usResult1 upwards, so an
exact comparison would not match them. That is unchanged.
Verification:
grep -rn 'strstr([^)]*)[ \t]*[><]' --include=*.cpp --include=*.h . # nothing
ninja -C build parse # pointer-ordering class gone: 32 -> 28 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>
STR8 is char*, so const STR8 is char* const: a const pointer to a mutable
string, which is not what any of these sixty parameters wanted. What they
wanted is const CHAR8*, a pointer to a string they will not write to, and
that is what all of them do.
Under the MSVC compatibility rules clang-cl applies, a string literal still
converts to char*, so most calls survive this mistake untouched. An expression
of type const char* does not, and Intro.cpp has thirteen of them:
inireader.ReadString("INTRO_BEGINNING", "INTRO_REBEL_CRDT",
no_defaults ? "" : "INTRO\\Rebel_cr");
The conditional is a const char*, not a literal, so it has nowhere to bind and
the call does not compile. The other two arguments are literals and are
accepted, which is why only the third one was ever reported.
The invariant: in these two files, every parameter spelled const STR8 becomes
const CHAR8*, and nothing else changes. No body assigns to one of them, so
none needed rewriting; STR8 input_buffer in the five-argument ReadString is
genuinely an output and keeps its type. Three commented-out lines still say
const STR8 and were left as they are.
Verification:
grep -n 'const[ \t]*STR8' Utils/INIReader.h Utils/INIReader.cpp
# three hits, all on lines beginning with //
git show HEAD -U0 | grep '^[+-][^+-]' |
sed -E 's/const[ \t]+(STR8|CHAR8\*)/const/g; s/^[+-]//' |
sort | uniq -c | awk '$1 % 2' # no output
ninja -C build parse # ReadString class gone: 45 -> 32 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>
DIRECTIVE_TEXT builds four enumerator names from one directive name:
#define DIRECTIVE_TEXT(id) RCDT_##id##, RCDT_##id##_EFFECT, ...
The last ## on the first name has nothing to paste onto but the comma that
follows, and RCDT_GATHER_SUPPLIES, is not a token. The other three pastes are
well formed; only this one runs off the end of the name.
MSVC concatenates the text and re-lexes it, so it recovers the identifier and
the comma and produces the right enumerators; clang diagnoses it. Removing the
stray ## gives the same ten enumerations, and MISSION_TEXT just below already
spells it this way.
Verification:
grep -rn '##[A-Za-z_]*##[,)]' --include=*.h --include=*.cpp . # nothing
ninja -C build parse # token-paste class gone: 55 -> 45 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>
DEBUG.H once wrapped its declarations in extern "C", and DEBUG.cpp wrapped the
matching definitions to agree. At some point the header's pair was commented
out, both the opening brace and the closing one:
/*
#ifdef __cplusplus
extern "C" {
#endif
*/
That had to happen, because the block would otherwise have swallowed
<stdexcept>, <list> and namespace sgp further down the header. The wrapper in
DEBUG.cpp was left behind, so seven variables are declared with C++ linkage in
the header and defined with C linkage in the source. That is ill-formed. MSVC
takes the linkage of the first declaration it sees, which is the header's, so
the mangled names still agree and the tree still links; clang refuses.
The header is the only place these seven are declared, so removing the
wrapper leaves everything with the C++ linkage it already had. Nothing here
needs to be callable from C.
A second such wrapper remains further down the file, inside #ifdef SGP_DEBUG.
Nothing in the build defines SGP_DEBUG and nothing it contains is declared in
a header, so it is neither compiled nor mismatched, and it is left alone.
Verification:
ninja -C build parse # language-linkage class gone: 60 -> 55 sites
ninja -C build -k 0 # Release, four applications, green
ninja -C build-debug -k 0 # Debug, four applications, green
The seven diagnostics sit on five lines, three of them sharing a single
declaration, and the parse target counts one site per line.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both of these bound a non-const reference to a temporary, which MSVC accepts
and clang does not, and which leaves the reference dangling once the full
expression ends.
PropertyContainer::initFromXMLFile and writeToXMLFile take their tag map by
non-const reference because the accessors on it insert default tag names as
they are asked for, so it cannot be const and it cannot be a temporary. All
eight call sites passed a freshly built one; they now declare it. Each keeps
its own object, so a map filled in by one call cannot leak into the next, the
same as when each call built its own temporary.
XMLWriter::writeToFile bound std::string& to what stringstream::str() returns
by value. Declared as a value it is initialised straight from the return with
no copy, and const because only c_str() and length() are asked of it.
Verification:
grep -rn 'PropertyContainer::TagMap()' . # nothing
ninja -C build parse # no i18n or XMLWriter sites remain
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>
PostalService.h names every type twice, once plainly and once as a reference:
RefToDestinationStruct is DestinationStruct&, RefToShipmentList is
ShipmentList&. That scheme was extended to the iterators, and there it does
not work. Every one of the seventeen uses looks like this:
RefToShipmentListIterator sli = _Shipments.begin();
begin() returns a value, so this binds a non-const reference to a temporary.
MSVC allows it as an extension; the temporary still dies at the end of the
full expression, which leaves sli referring to a dead object for the whole
rest of the function. It works only because nothing has reused the stack slot
yet. clang rejects it outright.
Iterators are already handles into their container, so a reference to one buys
nothing: the plain typedef says exactly what these seventeen variables want to
be. DESTINATION and SHIPMENT now take theirs by value, which changes nothing
for either, as both only dereference the iterator they are given.
Two of the reference typedefs, RefToDestinationDeliveryInfoTableIterator and
RefToDeliveryCallbackDataListIterator, had no uses at all. They are removed
rather than left behind, since their only possible use is the pattern this
commit removes.
Verification:
grep -rn 'RefTo[A-Za-z]*Iterator' --include=*.cpp --include=*.h . # nothing
ninja -C build parse # bind-to-temporary class gone: 91 -> 60 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>
A braced initialiser does not allow a narrowing conversion unless the value
is a constant expression the compiler can prove fits. Every site here holds a
runtime value: WORLD_COLS is the global guiWorldCols, the laptop screen
coordinates are built from iScreenWidthOffset, the sector limits come from the
externalised options. MSVC accepts all of it silently; clang rejects it, and
it is ill-formed.
Nothing about the generated code changes. MSVC was already performing these
conversions; the casts only say so out loud, at the 73 places where it was
happening implicitly.
The invariant: each site is wrapped in a static_cast to the element type the
initialiser already had, and nothing else is touched. No type is widened, no
expression is reassociated, no value is clamped or checked. static_cast rather
than a C-style cast so that a later reader can grep for the narrowings, and so
that none of these can quietly become a reinterpret_cast if a type changes.
Verification:
# every changed line differs only by inserted casts and their parentheses
git show HEAD -U0 | grep '^[+-][^+-]' |
sed -E 's/static_cast<[A-Za-z0-9_]+>//g; s/[()]//g; s/^[+-]//' |
sort | uniq -c | awk '$1 % 2' # no output
git show HEAD -U0 | grep -c '^+[^+]' # 38 lines, 73 casts
ninja -C build parse # narrowing class gone: 129 -> 91 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>
POPUP_OPTION copies the name it is given -- `this->name = *newName` into a
std::wstring member -- but its constructor took a std::wstring*, so 64 call
sites spelled the argument `&std::wstring( pStr )`. Taking the address of a
temporary is ill-formed, and clang refuses it outright:
error: taking the address of a temporary object of type 'std::wstring'
It happens to work under MSVC because the temporary outlives the call, dying
at the end of the full expression rather than before the copy. Nothing was
corrupt; the code was just spelling "pass me a string" in a way the language
does not allow.
A const reference says what these functions actually want, so the call sites
lose the &, and the two places that allocated a string purely to have an
address to pass -- POPUP_SUB_POPUP_OPTION's default constructor, which carried
a "TODO: possible memmory leak!" saying as much, and a "Dummy generator"
option -- stop leaking one.
Converted: POPUP_OPTION's constructor and setName, POPUP::addOption and
addSubMenuOption, and both POPUP_SUB_POPUP_OPTION constructors. The popupDef
family in popup_definition.* keeps its std::wstring* because it owns what it
is handed and stores the pointer; that is a different design and a different
change. The three calls it makes into the converted API now dereference.
Verification:
grep -rn '&std::wstring' --include=*.cpp --include=*.h . # nothing
ninja -C build parse # no popup_* or SkillMenu sites remain
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 color-shift opcode copies a run of palette entries from the previous
frame's palette to the current one. libsmacker rejected the copy when the
source and destination ranges overlapped, aborting the whole palette decode
and leaving the palette half-updated -- so every frame from the first such
opcode on rendered with a mix of the new and stale palette.
The overlap check is bogus: the copy reads from oldPalette, a snapshot taken
at the top of the function, and writes into s->palette, a separate buffer, so
overlapping ranges are harmless (and it is a memmove regardless). ffmpeg and
the original SMACKW32.DLL have no such check.
Several of the videos that ship with the game (Rebel_cr, Omerta, Prague) use
these overlapping copies, and showed washed-out colours with the previous
frame's palette bleeding through. Drop the overlap clause, keep the real
256-entry bounds checks.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drops the last piece of the proprietary Smacker SDK: SMACKW32.LIB, its
header sgp/SMACK.H, and with them the SMACKW32.DLL the game needed at
runtime. Bink is untouched and still needs binkw32.
The two libraries divide the work differently, so Cinematics.cpp changes
more than the call names. SMACKW32.DLL played a file: it kept the playback
clock (SmackWait), blitted converted 16bpp pixels into a surface we handed
it (SmackToBuffer) and pushed the audio to DirectSound itself. libsmacker
only decodes, so this module now:
- keeps the clock. Frames are advanced against GetTickCount() from the
moment the flic starts, skipping frames if the game fell behind, which
is what SmackWait/SmackDoFrame did internally.
- blits itself. Frames come out 8bpp palettised, so the palette is
converted with Get16BPPColor() and the frame is written into the frame
buffer, clipped to the screen. The Y-scaling flags are not handled;
none of the videos shipping with the game set them.
- plays the audio itself, through the SoundPlayFromBuffer() added earlier.
libsmacker hands out audio a frame at a time, so the whole track is
decoded up front, with the video track disabled to keep that pass cheap,
and given a WAV header so the sound module can take it.
libsmacker reads from memory, which lets the file be read straight out of
the VFS. That removes the dance where the .smk was first copied to a real
file under Temp/ because SmackOpen() could only open a path.
SmkInitialize() loses its window and screen size arguments, which only
existed for SmackBufferOpen(). SmkOpenFlic(), SmkSetBlitPosition() and
SmkGetFreeFlic() are no longer declared in the header; nothing outside the
module called them. SMKFLIC is now opaque, Intro.cpp only holds pointers.
The unused SMACK.H include in Cinematics Bink.cpp goes away as well.
Tested by running the Sirtech splash screen (SPLASHSCREEN.SMK, 640x480,
15 fps, one 44.1kHz 16 bit stereo track) under Wine: picture, palette,
placement and playback speed are right.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
refactor
Smacker video carries its audio as PCM that the video decoder produces
itself, so there is no file for the sound module to open. Add
SoundPlayFromBuffer(), which streams from a caller-owned memory buffer.
It deliberately does not go through the sample cache: a full length intro
video decodes to close to 20 MB of PCM, which does not fit in the cache's
memory budget and would evict everything else on the way to failing. The
channel bookkeeping already has a notion of a stream with no cache slot
behind it (uiSample == -1), which is what streamed files use, so the buffer
simply stays the caller's property for as long as the sound plays.
To share the parameter handling, everything SoundStartStream() did after
opening the stream moves unchanged into SoundStartOpenedStream().
No caller yet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
libsmacker is an open reimplementation of the parts of smackw32.dll needed
to get frames and audio out of an .smk file. Vendoring it here is the first
step to dropping the proprietary SMACKW32.LIB/SMACKW32.DLL dependency.
Nothing links against it yet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vfs_debug.h forward declares std::exception under #ifdef _MSC_VER. clang-cl
defines _MSC_VER too, and rejects the declaration:
vfs_debug.h(34,15): error: forward declaration of class cannot have a
nested name specifier
so any tool built on clang stops at the first file that reaches sgp/DEBUG.H,
which is nearly all of them. Guarding on __clang__ as well leaves the MSVC
build byte for byte identical and lets include-what-you-use parse the tree.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tiledef.h is included by 1900 objects, and each one was compiling a tentative
definition of gFullBaseTileValues rather than a declaration of it. The array
is defined once in TileDat.cpp, and every other global in the same block is
already declared extern; this one was missed.
MSVC accepts it and merges the tentative definitions, so nothing was visibly
broken. A conforming front end rejects it outright:
tiledef.h(180,12): error: definition of variable with array type needs
an explicit size or an initializer
which is what turned it up.
Verification: ninja -C build -k 0, all four applications link.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
to avoid having to ship a whole ja2.ini for languages that
currently don't
command-line /language:german still works, but INI file is
prioritized
format:
```
[Language]
LANGUAGE = ITALIAN
```
InitializeFileDatabase, the only reader of gGameLibaries[], is called
nowhere in the tree — VFS replaced this legacy loader. Delete the
7 #ifdef GERMAN/POLISH/DUTCH/ITALIAN/RUSSIAN/FRENCH/CHINESE entries
outright rather than runtime-selecting them.
NPC.h: two commented-out #if defined(CRIPPLED_VERSION)||defined(RUSSIAN)
triplets. AimMembers.cpp: two commented-out #ifdef POLISH character-remap
switch blocks. All already-dead. Verify grep for language macros outside
i18n/ now returns only unrelated hits (filename literal, dev-tool CLI flag,
stray comment, historical prose) — no #ifdef guards left.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
squashed into a single commit because doing it in steps that all
compiled involved a lot of boilerplate that is pointless to commit to
the repository. GH still has the PR for reference.
g_lang, MAX_MESSAGES_ON_MAP_BOTTOM, and GetLanguagePrefix() were
compile-time constants selected by the ENGLISH/GERMAN/... build
define. They're now runtime, defaulting to the same per-exe value the
define used to pick, and overridable at startup from [Ja2 Settings]
LANGUAGE in Ja2.ini.
-
XMLTacticalMessages is filled at runtime from NewTacticalMessages.xml,
never from compiled-in per-language data: all 8 per-language definitions
were the identical all-zero { L"" }. Delete them and define one shared
buffer in Utils/XML_Language.cpp (the loader) instead of pointer-rebinding
it like the static tables — this drops 9 dead 800KB zero-buffers (8
namespaced copies in LanguageStrings.cpp plus the standalone one) and
leaves no bind-ordering hazard for the XML load path.
-
ExportStrings.cpp privately recompiled one language's full text table
by #including the raw _<LANG>Text.cpp inside namespace Loc, keyed off
the exe-level ENGLISH/GERMAN/... compile macro (whichever the build
happened to select). That's redundant with the pointer globals every
other subsystem already uses (Text.h / LanguageStrings.cpp).
Drop the private copy; the unqualified table names in Loc::ExportStrings
now resolve to the global pointer externs, which BindLanguageStrings has
already rebound to the runtime g_lang by the time this runs (EXPORT_STRINGS
ini flag, checked after GetRuntimeSettings in sgp.cpp). gs_Lang (used only
by Loc::Translate for the Polish/Russian byte remap on raw .edt exports) is
now derived from g_lang via ToLocLanguage, so the export always matches
whatever language is actually active instead of a compile-time pick.
-
Editor/popupmenu.cpp and Strategic/Scheduling.cpp kept their own
call-site extern of gszScheduleActions as CHAR16[NUM_SCHEDULE_ACTIONS][20]
after the real definition changed into a rebindable pointer
(CHAR16 (*)[20], LanguageStrings.cpp). MSVC decays the outer array
dimension when mangling globals, so both declarations produce the same
symbol (?gszScheduleActions@@3PAY0BE@_WA) and the mismatch linked
silently -- but the array-typed TUs then indexed the 4-byte pointer
slot itself as string data, so the editor schedule popup and the map
schedule message text read garbage.
-
add text.def to be single-source of truth on symbol names and use it
.much simpler, less error prone if adding more strings
-
add pseudo interface for language state. MAX_SAGES_ON_BOTTOM must always
change in lockstep with g_lang. this isn't foolproof but better
---------
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Marco Antonio J. Costaandmajcosta
steph-pgandAsdow