Skip to content

refactor: S24.08 enforce top-down function ordering across all .c files#425

Merged
DavidCozens merged 1 commit into
mainfrom
refactor/s24-08-function-ordering
May 22, 2026
Merged

refactor: S24.08 enforce top-down function ordering across all .c files#425
DavidCozens merged 1 commit into
mainfrom
refactor/s24-08-function-ordering

Conversation

@DavidCozens

@DavidCozens DavidCozens commented May 22, 2026

Copy link
Copy Markdown
Owner

Purpose

Closes #423. Re-applies the documented function-ordering convention (_Create/_Destroy first, public funcs next, static helpers defined immediately beneath their first caller) across the project tree. The convention had drifted in pool-class *Static.c files and a handful of platform adapters — caught by CodeRabbit on PR #422 (SolidSyslogPosixAddressStatic.c::HandleFromIndex sat after _Destroy despite its first caller being _Create).

Change Description

Move-only refactor. No behaviour change.

Files touched (24):

  • Core/Source (5)SolidSyslogStatic.c, SolidSyslogMetaSdStatic.c, SolidSyslogUdpSenderStatic.c, SolidSyslogBlockStoreStatic.c, BlockSequence.c.
  • Platform (13) — Atomics, FatFs, FreeRtos (Address/Datagram/Mutex/StaticResolver/TcpStream), MbedTls, OpenSsl, Posix (Address/Datagram/TcpStream), Windows (AtomicCounter, WinsockAddress/Datagram/TcpStream). Files audited as compliant and left alone: PosixClock, WindowsClock, WinsockResolver.
  • Bdd/Targets (2)diskio.c (Semihosting helpers regrouped into call-graph order), FreeRTOS BDD main.c (main + four vApplication* OS-callback hooks lifted to the top, OnSet cluster reorganised into call-graph order, three forward decls added for SetEthernetIrqPriority/InteractiveTask/ServiceTask).
  • DEVLOG.md — entry documenting the audit method and the two convention clarifications gathered along the way.

Convention clarifications applied:

  • Vtable methods called by _Cleanup. When _Cleanup delegates to _Close (TlsStream / MbedTlsStream / WinsockTcpStream), _Close moved up beneath _Cleanup. When _Cleanup inlines its own close logic (PosixTcpStream), _Close stays at the end in API order.
  • main is a special case (per direction during review): entry-point first, other rules apply normally.

Helper-to-helper micro-ordering within a single caller's helper cluster (e.g. ordering of BreakDownFileTime / MicrosecondsFromFileTime / PopulateTimestamp under _GetTimestamp) was left alone — the strict reading would touch many compliant files for stylistic gain only.

Test Evidence

  • Local: cmake --preset debug && cmake --build --preset debug --target junit — all tests green (host tests + MbedTls integration tests in the cpputest-freertos container).
  • Local: clang-format --dry-run --Werror on changed files — clean.
  • CI will run the full matrix.

Pre-existing format violation in TlsStreamStatic.c and FatFsFileStatic.c (bool released = ... continuation indent) is out of scope — both files untouched by this PR.

Areas Affected

  • Source organisation only. No public-API or behaviour changes.
  • Diff is move-only: every change is a function definition (and sometimes its leading comment block) relocated within the same file. No semantic edits.
  • Tests/ explicitly out of scope per the issue body. Fakes were audited anyway and turned out to be compliant — no changes needed there.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor

    • Reorganized internal code structure across multiple platform and core components to follow project conventions for function placement and ordering. Functions and static helpers are repositioned for improved code organization and maintainability without affecting functionality.
  • Chores

    • Updated development documentation to record the code organization audit and conventions applied during this refactoring sweep.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@DavidCozens has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 52 minutes and 50 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b9668414-7e7c-4b37-90d7-8d3f8e1762da

📥 Commits

Reviewing files that changed from the base of the PR and between f864e26 and 1fc159b.

📒 Files selected for processing (24)
  • Bdd/Targets/FreeRtos/diskio.c
  • Bdd/Targets/FreeRtos/main.c
  • Core/Source/BlockSequence.c
  • Core/Source/SolidSyslogBlockStoreStatic.c
  • Core/Source/SolidSyslogMetaSdStatic.c
  • Core/Source/SolidSyslogStatic.c
  • Core/Source/SolidSyslogUdpSenderStatic.c
  • DEVLOG.md
  • Platform/Atomics/Source/SolidSyslogStdAtomicCounter.c
  • Platform/FatFs/Source/SolidSyslogFatFsFile.c
  • Platform/FreeRtos/Source/SolidSyslogFreeRtosAddressStatic.c
  • Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.c
  • Platform/FreeRtos/Source/SolidSyslogFreeRtosMutex.c
  • Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c
  • Platform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.c
  • Platform/MbedTls/Source/SolidSyslogMbedTlsStream.c
  • Platform/OpenSsl/Source/SolidSyslogTlsStream.c
  • Platform/Posix/Source/SolidSyslogPosixAddressStatic.c
  • Platform/Posix/Source/SolidSyslogPosixDatagram.c
  • Platform/Posix/Source/SolidSyslogPosixTcpStream.c
  • Platform/Windows/Source/SolidSyslogWindowsAtomicCounter.c
  • Platform/Windows/Source/SolidSyslogWinsockAddressStatic.c
  • Platform/Windows/Source/SolidSyslogWinsockDatagram.c
  • Platform/Windows/Source/SolidSyslogWinsockTcpStream.c
📝 Walkthrough

Walkthrough

This PR applies a codebase-wide function-ordering refactor (S24.08) that reorders static inline helpers and internal function definitions across Core, Platform, and Bdd/Targets modules to enforce the documented naming/layout convention: _Create/_Destroy first, then public functions, with helpers placed immediately before their first caller. No logic changes, feature additions, or behavioral modifications—purely reordering for maintainability and code navigation.

Changes

Function-Ordering Refactor Sweep

Layer / File(s) Summary
Platform abstraction helper reordering
Platform/Atomics/Source/SolidSyslogStdAtomicCounter.c, Platform/Windows/Source/SolidSyslogWindowsAtomicCounter.c, Platform/FreeRtos/Source/SolidSyslogFreeRtos*, Platform/Posix/Source/SolidSyslogPosix*, Platform/Windows/Source/SolidSyslogWinsock*, Platform/MbedTls/Source/SolidSyslogMbedTlsStream.c, Platform/OpenSsl/Source/SolidSyslogTlsStream.c
Static inline cast/validation helpers (_SelfFromBase, _IsFileDescriptorValid, _IsSocketValid, _HandleFromIndex) and lifecycle helpers (_Close, _Init, _Cleanup) are relocated earlier in atomics, address, datagram, TCP stream, and mutex implementations. All helper logic, vtable wiring, and stream close/read behavior remain unchanged.
Core logic helper reordering
Core/Source/BlockSequence.c, Core/Source/SolidSyslogBlockStoreStatic.c, Core/Source/SolidSyslogStatic.c, Core/Source/SolidSyslogMetaSdStatic.c, Core/Source/SolidSyslogUdpSenderStatic.c
Capacity-threshold checking and notification helpers, security-policy/block-sequence builders, instance population, and destroy/validation helpers are moved earlier in translation units to appear before their first use. Implementation logic unchanged.
Semihosting disk I/O helper reordering
Bdd/Targets/FreeRtos/diskio.c
ARM semihosting helpers (SemihostingOpen, Semihosting BKPT syscall, file operations Flen/Close/Seek/Write/Read) relocated earlier or later in the file according to call order. No FatFs disk API signatures or control flow changed.
FatFs file abstraction helpers
Platform/FatFs/Source/SolidSyslogFatFsFile.c
FatFsFile_SelfFromBase inline helper introduced, and FatFsFile_Close moved earlier with conditional socket/file close and state update logic preserved.
Main application initialization and task lifecycle reorganization
Bdd/Targets/FreeRtos/main.c
Forward declarations added; application entrypoint (main() UART init → IRQ priority → Plus-TCP setup → scheduler start) and FreeRTOS hooks moved earlier; configuration helpers (TryUpdateString, TryParseUInt) relocated; FATFS mount/format (EnsureFatFsMounted()) and discard policy mapping (MapDiscardPolicy()) consolidated; TeardownAll() reworked to set teardown/ready flags under lifecycle mutex, destroy SolidSyslog/SD/counter, tear down store/FAT, delay for task observation, then destroy resources in order. InteractiveTask() and ServiceTask() task initialization and periodic drain behavior unchanged.
Refactoring documentation
DEVLOG.md
New top entry dated 2026-05-22 documents the S24.08 refactoring sweep: standardized function-ordering convention applied across Core/Source, Platform/, and Bdd/Targets/; audit rule recorded; clarifications found (helper-to-helper call order, _Cleanup/_Close ordering); one deferred follow-up item noted.

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • DavidCozens/solid-syslog#423: This PR directly implements the S24.08 top-down function-ordering refactor policy described in the issue, moving helper definitions to appear before their first caller throughout the codebase.

Possibly related PRs

  • DavidCozens/solid-syslog#372: Both PRs modify the threshold-crossing helpers (BlockSequence_*Threshold*, BlockSequence_NotifyThresholdCrossed) in Core/Source/BlockSequence.c, with the main PR reordering those same helpers while the linked PR refactors their logic and naming.
  • DavidCozens/solid-syslog#378: Both PRs add/move FatFsFile_SelfFromBase cast helper in Platform/FatFs/Source/SolidSyslogFatFsFile.c as part of self/base cast-helper standardization.
  • DavidCozens/solid-syslog#213: The main PR's reordering of WinsockTcpStream_SelfFromBase and WinsockTcpStream_Close in Platform/Windows/Source/SolidSyslogWinsockTcpStream.c affects the Winsock TCP transport layer originally added in the linked PR.

Poem

A rabbit hops through files so neat,
Moving helpers to their seat—
Before they're called, in ordered place,
The code now flows with better grace. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.99% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: enforcing top-down function ordering convention across all C files, which is the primary refactoring objective.
Description check ✅ Passed The description fully addresses all required sections: purpose (closes #423 with clear context), detailed change description (move-only refactor with 24 files listed), test evidence (local tests passed, CI pending), and areas affected (source organization only, no API changes).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/s24-08-function-ordering

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

☀️   Quality Summary

   🚦   build-linux-gcc: 100% successful (✔️ 1296 passed, 🙈 2 skipped)
   🚦   build-freertos-host-tdd: 100% successful (✔️ 1518 passed, 🙈 2 skipped)
   🚦   build-linux-clang: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   🚦   sanitize-linux-gcc: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   🚦   integration-linux-openssl: 100% successful (✔️ 9 passed)
   🚦   integration-linux-mbedtls: 100% successful (✔️ 7 passed)
   🚦   integration-windows-openssl: 100% successful (✔️ 9 passed)
   🚦   bdd-linux-syslog-ng: 94% successful (✔️ 46 passed, 🙈 3 skipped)
   🚦   bdd-windows-otel: 90% successful (✔️ 44 passed, 🙈 5 skipped)
   🚦   bdd-freertos-qemu: No test results available
   🚦   build-windows-msvc: 100% successful (✔️ 1136 passed, 🙈 1 skipped)
   🚦   build-linux-tunable-override: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   ⚠️   Clang-Tidy: No warnings
   ⚠️   CPPCheck: No warnings


🚧   Error Messages

Quality Monitor Errors:
No matching report files found when using pattern '**/junit-bdd-freertos-qemu/TESTS-*.xml'! Configuration error for 'bdd-freertos-qemu'?

Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result.

Pure move-only refactor. Re-applies the documented function-ordering
convention (`_Create`/`_Destroy` first, public funcs next, static
helpers defined immediately beneath their first caller) across every
`.c` file under Core/Source, Platform/, and Bdd/Targets/. Tests/fakes
audited clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DavidCozens DavidCozens force-pushed the refactor/s24-08-function-ordering branch from f864e26 to 1fc159b Compare May 22, 2026 14:13
@github-actions

Copy link
Copy Markdown
Contributor

☀️   Quality Summary

   🚦   build-linux-gcc: 100% successful (✔️ 1296 passed, 🙈 2 skipped)
   🚦   build-freertos-host-tdd: 100% successful (✔️ 1518 passed, 🙈 2 skipped)
   🚦   build-linux-clang: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   🚦   sanitize-linux-gcc: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   🚦   integration-linux-openssl: 100% successful (✔️ 9 passed)
   🚦   integration-linux-mbedtls: 100% successful (✔️ 7 passed)
   🚦   integration-windows-openssl: 100% successful (✔️ 9 passed)
   🚦   bdd-linux-syslog-ng: 94% successful (✔️ 46 passed, 🙈 3 skipped)
   🚦   bdd-windows-otel: 90% successful (✔️ 44 passed, 🙈 5 skipped)
   🚦   bdd-freertos-qemu: 86% successful (✔️ 42 passed, 🙈 7 skipped)
   🚦   build-windows-msvc: 100% successful (✔️ 1136 passed, 🙈 1 skipped)
   🚦   build-linux-tunable-override: 100% successful (✔️ 1248 passed, 🙈 2 skipped)
   ⚠️   Clang-Tidy: No warnings
   ⚠️   CPPCheck: No warnings


Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result.

@DavidCozens DavidCozens merged commit ddbc81f into main May 22, 2026
21 checks passed
@DavidCozens DavidCozens deleted the refactor/s24-08-function-ordering branch May 22, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S24.08: enforce top-down function ordering across all source files

1 participant