Repository navigation
Reduce core surface: dead platforms, extras library, DS_* deprecations, snake_case files (#50) - #58
Merged
Merged
Conversation
…stages 1 and 4) Remove the Xbox 360, PS3, PS4, Vita and Native Client remnants: the empty *Includes.h/.cpp stubs, Samples/nacl_sdk, the pre-RakNetSocket2 socket.h / RakNetSocket.cpp (no consumers, only commented-out console code), and every __native_client__ / _PS3 / _PS4 / SN_TARGET_* preprocessor island. The PS3/Native Client fields on SocketDescriptor and RNS2_BerkleyBindParameters go with them; RNS2_Windows_Linux_360 becomes RNS2_Windows_Linux. ID_XBOX_* message identifiers stay as reserved values so wire values do not shift. Add MAFIANET_DISABLED_FEATURES and MAFIANET_MINIMAL so the _RAKNET_SUPPORT_<Name> flags from NativeFeatureIncludes.h can be set from CMake (PUBLIC on the library targets, unknown names rejected), document them in building.rst, and add a linux-minimal CI job that builds the all-plugins-off configuration.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (56)
💤 Files with no reviewable changes (36)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR adds CMake options to disable selected or all optional plugins and adds a Linux minimal-build job. It also removes legacy platform code and socket interfaces, updates shared networking paths, and documents the changes. ChangesBuild Configuration and Platform Cleanup
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Merge Risk: ⚪ Minimal · up to This PR removes obsolete platform code and adds options for minimal builds. No concrete merge-blocking risk was found; consumers must rebuild because SocketDescriptor changed. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 14 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the build at dawn, Comment |
…#50 stages 2 and 3) Stage 2: move EmailSender, HTTPConnection/HTTPConnection2, TelnetTransport, RakNetTransport2, ConsoleServer, CommandParserInterface, RakNetCommandParser, LogCommandParser and PacketConsoleLogger out of the core into a MafiaNetExtras static library (MAFIANET_BUILD_EXTRAS, forced on by MAFIANET_BUILD_SAMPLES). Headers and include paths are unchanged; samples link the extras target. Stage 3: DS_BinarySearchTree, DS_QueueLinkedList, DS_HuffmanEncodingTreeFactory, DS_OrderedChannelHeap and DS_BytePool have no in-tree consumer; they now emit a deprecation #pragma message and are scheduled for removal. Header naming: every public header now carries the PascalCase name of what it declares, matching its .cpp (peerinterface.h -> RakPeerInterface.h, string.h -> RakString.h, types.h -> RakNetTypes.h, ...). The umbrella mafianet/mafianet.h stays. Old names remain as forwarding headers with a #pragma message for one release cycle, except aliases.h, version.h and gettimeofday.h, whose new names differ only by case. The lowercase .cpp files were renamed to match.
Segfaultd
changed the title
Follow-up to the header rename: MafiaNet dropped the Rak/RakNet prefix on purpose, so the renamed headers use the bare PascalCase name (PeerInterface.h, String.h, Types.h, ...). The lowercase shims whose new name differs only by case are removed since they cannot coexist on case-insensitive file systems.
Segfaultd
force-pushed
the
chore/drop-dead-platforms
branch
from
October 8, 2026 09:00
b3055fd to
d3cc2ae
Compare
Every header and source is now lowercase snake_case with no RakNet prefix (bit_stream.h, peer_interface.h, voice.h, cc_udt.h), and each .cpp matches its header. Class names are unchanged. Library headers include siblings only as "mafianet/<name>.h" so string.h, time.h and assert.h cannot shadow the C headers; a CI step enforces both rules. The remaining lowercase forwarding shims are removed since the real files now carry those names.
Segfaultd
changed the title
The snake_case pass rewrote the sample's SOURCES line too, although that file belongs to the sample, not the library, and was not renamed.
Pushes to master no longer rebuild or retest, since master only changes through a PR that already ran Build & Test. A release tag triggers the packaging workflow and the documentation deploy, neither of which runs tests. A new push to a PR cancels that PR's in-flight run.
_findnext copied each entry name with a count equal to the destination size. The strncpy_s shim then read one byte past dirent::d_name to decide whether the copy fit, saw garbage on Linux, returned ERANGE and an empty name. The empty name slipped past the "."/".." filter and AddFilesFromDirectory recursed into "dir//" forever, allocating until the OOM killer stopped it; that is the 600 s timeout of DirectoryDeltaTransferStreaming on Linux CI. _findnext now copies with _TRUNCATE, and the shim reads at most count bytes of the source and rejects a copy that would not fit, as MSVC does, instead of writing the terminator one byte past the destination. Unit tests pin both.
This was referenced Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #50. All four stages, plus a header-naming cleanup.
Stage 1: dead platforms removed
Xbox 360, PS3, PS4, PS Vita and Google Native Client. Their
*Includes.hheaders were already empty stubs; what remained was#ifdefislands and commented-out console code.XBox360Includes.h,PS3Includes.h,PS4Includes.h,VitaIncludes.h,PS4Includes.cpp,VitaIncludes.cpp, andSamples/nacl_sdk/.socket.h/RakNetSocket.cpp: the pre-RakNetSocket2socket class. The.cppwas one big comment and the header had zero includers.__native_client__,_PS3/__PS3__/SN_TARGET_PS3,_PS4andSN_TARGET_PSP2branch (viaunifdef), plus the commented-out blocks that referenced them.RakNetSocket2_Windows_Linux_360.cppintoRakNetSocket2_Windows_Linux.cpp;RNS2_Windows_Linux_360→RNS2_Windows_Linux.SocketDescriptor::remotePortRakNetWasStartedOn_PS3_PSP2,SocketDescriptor::chromeInstance,RNS2_BerkleyBindParameters::remotePortRakNetWasStartedOn_PS3_PS4_PSP2and the_PP_Instance_macro. ABI note:SocketDescriptorshrinks, so consumers must rebuild.ID_XBOX_360_*/ID_XBOX_LOBBYare kept as reserved enum values so wire values do not shift.Stage 2: extras library
EmailSender,HTTPConnection,HTTPConnection2,TelnetTransport,RakNetTransport2,ConsoleServer,CommandParserInterface,RakNetCommandParser,LogCommandParserandPacketConsoleLoggermove out of the core intoMafiaNetExtras(MafiaNet::MafiaNetExtras), a static library built with-DMAFIANET_BUILD_EXTRAS=ON(off by default, forced on byMAFIANET_BUILD_SAMPLESsince nine samples use them). Headers stay underinclude/mafianet/, so include paths are unchanged; only the link line changes. The target links the corePUBLIC, so linking it alone is enough. Nothing in the core referenced these classes. Installed and exported alongside the core targets.Stage 3:
DS_*auditConsumer counts over
Source/,Tests/,Samples/,DependentExtensions/:ds_binary_search_tree.h,ds_huffman_encoding_tree_factory.h,ds_ordered_channel_heap.handds_byte_pool.hhave zero consumers;ds_queue_linked_list.his used only byds_binary_search_tree.h. All five now emit a#pragma messageon inclusion (silenced byMAFIANET_SILENCE_DEPRECATED_INCLUDES) and are scheduled for removal in the next minor. Everything else has at least one real consumer (ds_range_list/ds_bplus_treein the reliability layer,ds_weighted_graph/ds_treein ConnectionGraph2, etc.) and stays.Stage 4: minimal build configuration
-DMAFIANET_DISABLED_FEATURES="ReplicaManager3;RPC4Plugin"compiles out named plugins;-DMAFIANET_MINIMAL=ONcompiles out all 37 optional plugins. Names are validated againstMAFIANET_OPTIONAL_FEATURESinSource/CMakeLists.txt. Defines arePUBLICso consumers see the same header configuration.docs/getting-started/building.rst("Minimal builds", "The extras library"), changelog "Unreleased" section, CLAUDE.md.linux-minimalCI job builds static + shared withMAFIANET_MINIMAL=ON;linux-nativenow also builds the extras target.File naming: snake_case
The include directory mixed PascalCase (
ReliabilityLayer.h) with lowercase names inherited from SLikeNet (peer.h,string.h), and.cppfiles did not match their headers (peer.h↔RakPeer.cpp). Every file underSource/is now lowercasesnake_casewith no RakNet prefix, and each.cppmatches its header:BitStream.hbit_stream.hReliabilityLayer.h/.cppreliability_layer.h/.cpppeerinterface.h/RakPeer.cpppeer_interface.h/peer.cppDS_List.h,DS_BPlusTree.hds_list.h,ds_bplus_tree.hHTTPConnection2.h,UDPForwarder.h,RPC4Plugin.hhttp_connection2.h,udp_forwarder.h,rpc4_plugin.hRakVoice.h,CCRakNetUDT.h,RakNetSocket2_Berkley.cppvoice.h,cc_udt.h,socket2_berkley.cpp_FindFirst.hfind_first.h270 files renamed, 520 files had references rewritten (library, tests, samples, extensions, docs, README, CLAUDE.md). Class names are unchanged.
mafianet/mafianet.hand the already-lowercasecrypto/headers stay as they are.No forwarding shims. Almost every rename differs from the old name only by case, so shims cannot coexist on Windows or macOS. On those platforms the old spellings still resolve; on Linux consumers update their includes once. The changelog says so.
Include style, enforced. Several library headers now share a name with a C header (
string.h,time.h,assert.h,alloca.h). A bare#include "string.h"insideinclude/mafianet/would pick the library's file. All sibling includes inside the library were therefore normalized to"mafianet/<name>.h", and a new CI step fails on any uppercase file name underSource/or any bare quoted include of a library header. The only bare includes left are inside comments, the optional user-suppliedCustomPacketIdentifiers.h, and the externalcat/headers.Measurements (macOS arm64, Release, clean build of
MafiaNetStatic, 3 runs each)libMafiaNetStatic.aMAFIANET_MINIMAL=ONStage 1 removes ~1,900 lines but changes neither compile time nor size: that code was already preprocessed out on every supported platform. The minimal configuration cuts CPU time by ~34% and static size by 61%. Stage 2 moves ten translation units out of the default core build on top of that (not re-measured separately; the extras
.ais produced alongside when enabled).Bug fix found along the way: Linux directory walks never terminated
DirectoryDeltaTransferStreaming.LargeFilesAreWrittenChunkByChunkhit the 600 s timeout on every Linux CI run, on master as well. Reproducing in Docker with RSS sampling and gdb showed it was not a hang but the OOM killer:FileList::AddFilesFromDirectoryrecursed intooutput//////..forever at ~70 MB/s. Cause:_findnextcopied entry names withstrncpy_s(name, d_name, 512), a count equal to the destination size; the shim then readd_name[512], far past the 256-bytedirentfield, to decide whether the copy fit, saw garbage on Linux and returned an empty name, which defeated the"."/".."filter. macOS happened to read a zero there. The shim also wrote its terminator atdest[size]whencount == size. Both fixed (_TRUNCATEin_findnext; the shim now reads at mostcountbytes and rejects a copy that would not fit, as MSVC does), with unit tests for the shim bounds and the walk's termination.CI changes
Build & Test runs on pull requests only (plus
workflow_dispatch), with a new push to a PR cancelling its in-flight run. Pushes to master trigger nothing; av*tag triggers the release packaging workflow and the docs deploy, neither of which runs tests. A new step fails on uppercase file names underSource/or bare quoted includes of library headers.Verification
MAFIANET_BUILD_EXTRAS=ON: 211/211 unit and 65/65 integration pass in both.MafiaNetExtras.MAFIANET_MINIMAL=ONbuilds both library targets; an unknown name inMAFIANET_DISABLED_FEATURESfails at configure.SessionConfigLive.StaticExchangeDeliversBothPayloadsfails intermittently on master too (2/10 runs with master's own binary); CI's--repeat until-pass:3absorbs it and it deserves its own issue.Follow-ups
ds_*headers in the next minor.SessionConfigLive.StaticExchangeDeliversBothPayloadsflake.