Skip to content

Cleanup pass: dead code, needless copies, and a few small fixes - #465

Open
marekl11 wants to merge 1 commit into
getnamo:mainfrom
marekl11:chore/cleanup-and-small-fixes
Open

Cleanup pass: dead code, needless copies, and a few small fixes#465
marekl11 wants to merge 1 commit into
getnamo:mainfrom
marekl11:chore/cleanup-and-small-fixes

Conversation

@marekl11

Copy link
Copy Markdown
Contributor

Mostly a cleanup pass over the plugin's own code (ThirdParty untouched), with a few small bugs I ran into while doing it. No behaviour changes beyond the fixes listed below.

Cleanup

  • ToSIOMessage copied the whole FJsonObject::Values map on every object emit — auto on a TMap deduces to a copy. Also made the range-for loops take their pairs by reference, and StdString/FStringFromStd take const& instead of by value.
  • Dropped [&] from ten lambdas that already capture everything they use explicitly. The ones that genuinely reach for a member are left alone, so it's now visible which those are.
  • Removed the unused condensed-writer typedefs and their two includes, a commented-out declaration in the module, a dead CastField stub in SocketIOFunctionLibrary, and two #pragma endregion with no matching region.
  • ReplaceJsonValueNamesWithMap was serializing the whole subtree to a string at every object level to feed two commented-out log lines.

Fixes

  • Break Json is unusable. All three sites resolve the pin class via StaticLoadObject("class'SIOJPlugin.SIOJJsonObject'") — a module and class name from before the module split. The load returns null, Compile() dereferences it, and compiling a Blueprint containing the node takes the editor down. Now uses USIOJsonObject::StaticClass(); SIOJson is already a public dependency of that module.
  • Conv_CompactBytesToTransforms / Conv_CompactPositionBytesToTransforms write past the array. Both size a float view for Num()/4 floats and then memcpy Num() bytes into it before the divisibility check. Any input that isn't a multiple of four writes the trailing bytes out of bounds, and these are Blueprint-callable on network-supplied bytes. Moved the length check ahead of the copy.
  • SplitFullPath's backslash fallback can't match. It splits on TEXT("\\\\"), which is the two-character string \, so a Windows-style path never splits and both out params keep whatever the caller had — SaveBytesToPath then fails on the empty path.
  • EncodeJsonToSingleString does nothing. Both Replace calls discard their result (Replace returns a new string), and the second searches for LINE_TERMINATOR while its comment says "remove tabs" — once assigned it would have inserted a tab per line break.
  • GetBinaryField is missing two guards. No JsonObj.IsValid() check, unlike every other accessor in the file, and it logs a type mismatch without returning. FJsonValueBinary::IsBinary is also true for array, object and null values, so the unchecked downcast below reads a TArray member that isn't there.
  • ToSIOMessage built std::string(nullptr, 0) for an empty binary value.
  • ENABLE_CUPRECISE_TIMER was #defined unconditionally, so Tock's #else branch could never compile and the off mode didn't exist. Wrapped in #ifndef.

One naming change

FSocketIONative::kUnlimitedReconnectionAttempts. The component initialised MaxReconnectionAttempts from a float literal (-1.f) into an int32, the native used a bare -1 into a uint32, and the header said "0 = infinity" — but sio compares m_reconn_made < m_reconn_attempts on an unsigned, so 0 means zero attempts and only the -1 wraparound made the default unlimited. Nothing changes at runtime; it just says what it means, so a later tidy-up doesn't turn reconnection off.

Happy to split any of this out if you'd rather take it in pieces.

Mostly hygiene, plus a handful of small bugs found alongside it.

Cleanup:
- SIOMessageConvert: ToSIOMessage copied the whole FJsonObject::Values map
  per object emit (`auto` on a TMap); the two string converters took their
  arguments by value; the condensed-writer typedefs and their two includes
  were unused; the range-for loops copied their pairs.
- SocketIONative / SocketIOClientComponent: dropped `[&]` from ten lambdas
  that capture every name they use explicitly, so it is clearer which
  callbacks really do reach for a member. Left the ones that do alone.
- Removed a commented-out declaration, a dead `CastField` stub in
  SocketIOFunctionLibrary, and two `#pragma endregion` in SIOJConvert with
  no matching region.
- SIOJConvert::ReplaceJsonValueNamesWithMap serialized the whole subtree to
  a string at every object level to feed two commented-out log lines.

Fixes:
- Break Json is unusable: all three sites resolve the pin class with
  StaticLoadObject("class'SIOJPlugin.SIOJJsonObject'"), a module and class
  name from before the module split. The load returns null and Compile()
  dereferences it, so compiling a Blueprint containing the node crashes the
  editor. Uses USIOJsonObject::StaticClass() now (SIOJson is already a
  public dependency of that module).
- Conv_CompactBytesToTransforms / Conv_CompactPositionBytesToTransforms
  size a float view for Num()/4 floats and then memcpy Num() bytes into it
  before checking divisibility, so any input that is not a multiple of four
  writes past the end. These take network-supplied bytes from Blueprint.
  Length check moved ahead of the copy.
- CUFileComponent/CUFileSubsystem::SplitFullPath fell back to splitting on
  TEXT("\\\\") - a two-character string - so a Windows-style path never
  split and both out params were left untouched. SaveBytesToPath then
  failed on the empty path.
- USIOJsonObject::EncodeJsonToSingleString discarded both Replace results
  (Replace returns a new string), and the second searched for
  LINE_TERMINATOR while claiming to remove tabs.
- USIOJsonObject::GetBinaryField had no JsonObj.IsValid() guard, unlike
  every other accessor in the file, and logged a type mismatch without
  returning - FJsonValueBinary::IsBinary is also true for array, object and
  null values, and the downcast below it then reads a TArray member that is
  not there.
- ToSIOMessage built std::string(nullptr, 0) for an empty binary value.
- ENABLE_CUPRECISE_TIMER was #defined unconditionally, so the #else branch
  in Tock could never compile and the off mode did not exist. Wrapped in
  #ifndef.

Also named FSocketIONative::kUnlimitedReconnectionAttempts. The component
initialized MaxReconnectionAttempts from a float literal (-1.f) into an
int32, the native used a bare -1 into a uint32, and the header said
"0 = infinity" - but sio compares m_reconn_made < m_reconn_attempts on an
unsigned, so 0 means zero attempts and only the wraparound made the default
unlimited. Behaviour is unchanged; it just says what it means now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants