Skip to content

Commit 0b3df68

Browse files
kraenhansenclaude
andauthored
fix(host): compile out log_debug in release (NDEBUG) builds (#431)
* fix(host): compile out log_debug in release (NDEBUG) builds `log_debug`'s per-addon diagnostic chatter (library found/loaded, symbol resolution, ...) was firing unconditionally on every addon load, including in shipped release builds - unwanted logcat/os_log output plus string-formatting cost on a startup path. log_debug is now an inline no-op declared in Logger.hpp when NDEBUG is defined (set by CMake's Release/MinSizeRel/RelWithDebInfo configurations, and by Xcode's Release configuration by default), mirroring React Native's own dev/release logging split. Logger.cpp's real definition is compiled only outside of NDEBUG. log_warning/log_error are untouched and keep firing in every build type, including RelWithDebInfo. This is compile-time only, not the "compile-time default with runtime override" the issue floats as the ideal: there's no existing runtime config plumbing (env var, JS-settable flag, ...) in this codebase to hook an override into, so adding one would mean inventing new plumbing rather than reusing something established. Shipping the safer compile-time-only guard now per the issue's own fallback. Closes #420 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm * docs: keep inline comments brief, trim the Logger ones Adds a `.claude/CLAUDE.md` section: default to no inline comment, and keep the ones that survive to a line or two. Rationale, rejected alternatives and change narration go in the PR description, which is where a `git blame` leads anyway and which does not go stale as the surrounding code moves. Applies it to the log_debug/NDEBUG comments this PR added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmbKZsRagnasxGVXtnCLoF --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d9ab417 commit 0b3df68

4 files changed

Lines changed: 49 additions & 1 deletion

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"react-native-node-api": patch
3+
---
4+
5+
Stop emitting `log_debug`'s per-addon diagnostic chatter (library
6+
found/loaded, symbol resolution, ...) in release builds. It is now compiled
7+
out in `NDEBUG` builds (CMake's `Release`/`MinSizeRel`/`RelWithDebInfo`
8+
configurations, and Xcode's default `Release` configuration), mirroring React
9+
Native's own dev/release logging split. `log_warning` and `log_error` are
10+
unaffected and keep firing in every build type.

.claude/CLAUDE.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,33 @@ Guidance specific to Claude Code sessions that open pull requests against this
44
repository (including automated/scheduled sessions). See the root `CLAUDE.md`
55
and `AGENTS.md` for everything else.
66

7+
## Keep inline comments very brief — put the color in the PR description
8+
9+
Default to **no comment at all**. Write one only when it carries knowledge a
10+
reader cannot get from the code itself plus a `git blame` pointing at the PR
11+
that introduced it. When you do write one, keep it to a line or two.
12+
13+
Rationale, rejected alternatives, benchmark numbers, "we tried X and it
14+
didn't work", links to upstream issues, and anything that reads as a history
15+
lesson belong in the **PR description** (and, where user-facing, the
16+
changeset) — not in the source. Those places are where a reader who has
17+
already found the line via `git blame` will end up anyway, and they don't
18+
have to be maintained as the code around them changes.
19+
20+
Concretely, do not write comments that:
21+
22+
- restate what the next line already says;
23+
- explain why an alternative implementation was _not_ chosen;
24+
- narrate the change (`// now compiled out in release builds`) — that is a
25+
commit message, and it goes stale the moment the code moves;
26+
- document a well-known toolchain fact (e.g. what `NDEBUG` means) that a
27+
reader can look up.
28+
29+
Comments that _do_ earn their place: a non-obvious constraint the compiler or
30+
platform imposes, a workaround with the exact condition that makes it
31+
removable (see the upstream-fix guidance in `AGENTS.md`), or a subtle
32+
invariant a future edit could silently break.
33+
734
## Attach CI labels when you open a PR
835

936
`.github/workflows/check.yml`'s `pull_request` trigger only fires on

packages/host/cpp/Logger.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,15 @@ void log_message_internal(LogLevel level, const char *format, va_list args) {
6363

6464
namespace callstack::react_native_node_api {
6565

66+
#ifndef NDEBUG
6667
void log_debug(const char *format, ...) {
67-
// TODO: Disable logging in release builds
6868
va_list args;
6969
va_start(args, format);
7070
log_message_internal(LogLevel::Debug, format, args);
7171
va_end(args);
7272
}
73+
#endif
74+
7375
void log_warning(const char *format, ...) {
7476
va_list args;
7577
va_start(args, format);

packages/host/cpp/Logger.hpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,16 @@
33
#include <string>
44

55
namespace callstack::react_native_node_api {
6+
7+
// Inline (rather than a no-op in Logger.cpp) to let the optimizer drop the
8+
// argument evaluation at every call site.
9+
#ifdef NDEBUG
10+
inline void log_debug(const char *, ...) {}
11+
#else
612
void log_debug(const char *format, ...);
13+
#endif
14+
715
void log_warning(const char *format, ...);
816
void log_error(const char *format, ...);
17+
918
} // namespace callstack::react_native_node_api

0 commit comments

Comments
 (0)