Wrap Log() with #if DEBUG to skip string construction in release builds#310
Wrap Log() with #if DEBUG to skip string construction in release builds#310vivekjyani merged 2 commits intomainfrom
Conversation
VishnuBishnoi
left a comment
There was a problem hiding this comment.
Correct and effective change for the current codebase. A few observations on the implementation and a pre-existing bug worth noting while this file is open.
Why this works for the current codebase
All 175 Log() call sites live inside JoyfillUI itself — zero external callers. Release builds with WMO (the Xcode default, enabled under Compilation Mode → Whole Module) inline the now-empty Log() body across the entire module, which allows the optimizer to eliminate every call site — including string-interpolation argument expressions. The stated goal is achieved.
Suggestion: @autoclosure for stronger guarantee (non-blocking)
Since Log() is public, an external caller in a different module would not benefit from the #if DEBUG body guard — WMO cannot inline across module boundaries, so the string arguments would still be evaluated at the call site before entering the empty function. The idiomatic Swift fix for this is an @autoclosure parameter (same pattern used by assert, precondition, and fatalError):
public func Log(
_ message: @autoclosure () -> String,
type: LogType = .info,
function: String = #function,
file: String = #file,
line: Int = #line
) {
#if DEBUG
JoyfillLogger.shared.log(message(), type: type, function: function, file: file, line: line)
#endif
}This defers string construction to the closure; in release builds the body is compiled out, so the closure is never called and no string is built — regardless of WMO or module boundaries. Recommend doing this in a follow-up.
Pre-existing bug in log() — dead #else branch (out of scope but file is being touched)
The nested #if DEBUG / #else at ~line 50 inside log() has an unreachable #else branch:
#if DEBUG // outer guard
print(logMessage)
if type == .error {
#if DEBUG // always true here — we are already in #if DEBUG
if NSClassFromString("XCTest") == nil {
fatalError(logMessage)
}
#else
print("Critical error logged: \(logMessage)") // ← NEVER executes
#endif
}
#endifThe #else branch was presumably intended for production error logging, but it is nested inside the outer #if DEBUG, so it can never be reached. The misleading // Print to console in both debug and release builds comment on the outer guard compounds this. Recommend addressing in a follow-up:
#if DEBUG
print(logMessage)
if type == .error, NSClassFromString("XCTest") == nil {
fatalError(logMessage)
}
#else
if type == .error {
print("Critical error logged: \(logMessage)")
}
#endifApproving — the change is correct, minimal, and solves the immediate problem. The two items above are follow-up improvements, not blockers.
Context
A review suggestion flagged that the public
Log()function inJoyfillLogger.swiftwas still calling intoJoyfillLogger.shared.log()in release builds — causing unnecessary string interpolation andNSStringpath extraction on every call, even though thelog()method body already had#if DEBUGaroundprint. For high-frequency call sites likeonChange, this adds small but avoidable overhead in production.What Changed
File:
Sources/JoyfillUI/Extensions/JoyfillLogger.swiftWrapped the body of the public
Log()global function with#if DEBUGso that in release builds, the entire function body is compiled out — no function dispatch toJoyfillLogger.shared.log(), no string construction, zero overhead.Why This Approach
We chose to wrap at the
Log()call site rather than inside thelog()method becauseLog()is the single public entry point used across the codebase. This keeps the guard at the outermost layer and avoids even the cost of calling into thelog()method in release builds. The existing#if DEBUGinsidelog()is now redundant but left in place as a safety net.Screenshots / Video
N/A — no visual change. This is a compile-time optimization only.
Public API Impact
No public API change.
Log()remains available with the same signature. No documentation update needed.Test Coverage
No new tests added — this is a compile-time directive change that doesn't alter runtime behavior in debug builds. Existing logging behavior is unchanged.
Notes for Reviewer
#if DEBUGinsidelog()method (line 47) is now redundant but harmless — can be cleaned up in a follow-up if desired.