Skip to content

refactor(ini): Simplify some INI code#2541

Merged
xezon merged 1 commit intoTheSuperHackers:mainfrom
bobtista:bobtista/refactor/simplify-ini-loading-lookup
Apr 6, 2026
Merged

refactor(ini): Simplify some INI code#2541
xezon merged 1 commit intoTheSuperHackers:mainfrom
bobtista:bobtista/refactor/simplify-ini-loading-lookup

Conversation

@bobtista
Copy link
Copy Markdown

@bobtista bobtista commented Apr 5, 2026

Three focused cleanups to INI.cpp:

  • isValidINIFilename: replace manual character-by-character .ini extension check with AsciiString::endsWithNoCase, which is already used for the same purpose in loadFileDirectory

  • loadDirectory: remove a try/catch that only rethrew — identical behavior to no try/catch since there was no cleanup or transformation in the catch body

  • scanLookupList: remove unreachable found = true; break; lines that appeared after a return statement

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 5, 2026

Greptile Summary

This PR makes three independent dead-code and redundancy cleanups to INI.cpp. The manual three-character extension check in isValidINIFilename is replaced with the free template function endsWithNoCase(filename, ".ini") from stringex.h, which is both cleaner and strictly more correct (the old code did not require the preceding dot). The no-op try { ... } catch (...) { throw; } wrapper in loadDirectory is removed with no behavioral change. The unreachable found = true; break; statements after an unconditional return in scanLookupList are deleted along with the now-unused Bool found variable. No regressions are introduced by any of the three changes.

Confidence Score: 5/5

Safe to merge — all three changes are correct refactors with equivalent or strictly better behavior.

All changes eliminate dead or redundant code. The only subtle semantic shift (isValidINIFilename now requires the dot before 'ini') is strictly more correct, and the function appears to have no live call sites. No P0 or P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/INI/INI.cpp Three cleanups: replace manual extension check with endsWithNoCase, remove no-op try/catch, remove unreachable post-return dead code

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[isValidINIFilename] -->|Old| B["Manual check: last 3 chars == 'ini' (no dot)"]
    A -->|New| C["endsWithNoCase(filename, \".ini\") — includes dot, more correct"]

    D[loadDirectory] -->|Old| E["try { ... } catch(...) { throw; } — no-op"]
    D -->|New| F["Direct body — identical behavior, no wrapper"]

    G[scanLookupList] -->|Old| H["return lookup->value; found=true; break; — unreachable"]
    G -->|New| I["return lookup->value; — dead code removed"]
Loading

Reviews (3): Last reviewed commit: "refactor(ini): simplify INI file loading" | Re-trigger Greptile

@bobtista bobtista force-pushed the bobtista/refactor/simplify-ini-loading-lookup branch from d4487b8 to 00b9170 Compare April 5, 2026 23:45
@bobtista bobtista changed the title refactor(ini): simplify INI loading and optimize block type lookup refactor(ini): simplify INI file loading Apr 5, 2026
@bobtista
Copy link
Copy Markdown
Author

bobtista commented Apr 5, 2026

Greptile Summary

Four focused cleanups to INI.cpp: isValidINIFilename now uses endsWithNoCase(".ini") which is stricter than the old check (the old code accepted strings like xINI without a dot); the no-op try/catch in loadDirectory is removed; findBlockParse switches to binary search over the already-sorted theTypeTable; and unreachable dead code (found = true; break;) after return statements in scanLookupList is removed. All four changes are verified correct.

Confidence Score: 5/5

Safe to merge — all changes are correct refactors with no behavioral regressions

All four changes are verified correct: endsWithNoCase handles short strings safely, theTypeTable is sorted in strcmp order, the try/catch removal is semantically equivalent, and the dead-code removal is trivially safe. The only finding is a P2 style note about documenting the sort requirement for the binary search table.

No files require special attention

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/INI/INI.cpp Four clean refactors: stricter .ini filename validation, no-op try/catch removal, linear-to-binary-search for findBlockParse (table verified sorted), and dead-code removal after return statements

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[findBlockParse: token] --> B[Binary search theTypeTable]
    B --> C{strcmp result?}
    C -- zero: match --> D[Return parse function pointer]
    C -- negative: table < token --> F[lo = mid + 1]
    C -- positive: table > token --> E[hi = mid]
    E --> B
    F --> B
    B -- lo >= hi: not found --> G[Return nullptr]
Loading

Loading
Prompt To Fix All With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/INI/INI.cpp
Line: 340-354

Comment:
**Binary search requires sorted table**

The binary search is correct as-is, but `theTypeTable` has no documentation requiring it to remain sorted. A future entry inserted in the wrong position would silently return `nullptr` for valid tokens, breaking INI parsing for those block types without any compile-time or run-time diagnostic. Adding a comment near the table definition stating that entries must be kept in `strcmp` (case-sensitive ASCII) order would prevent this silent failure mode.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "refactor(ini): Simplify INI loading and ..." | Re-trigger Greptile

Fair, there's no guarantee for the sorting, and we could add an assert and/or a comment, but this is a small search and only happens once - not really worth changing, I reverted it.

Copy link
Copy Markdown

@Skyaero42 Skyaero42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me

@xezon xezon added the Refactor Edits the code with insignificant behavior changes, is never user facing label Apr 6, 2026
@xezon xezon changed the title refactor(ini): simplify INI file loading refactor(ini): Simplify some INI code Apr 6, 2026
@bobtista bobtista force-pushed the bobtista/refactor/simplify-ini-loading-lookup branch from 00b9170 to 86c4901 Compare April 6, 2026 17:17
@xezon xezon merged commit fb77779 into TheSuperHackers:main Apr 6, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants