From 7fcd7c3f2261a273b5ffbd34883be24b271d6edd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 07:29:25 +0000 Subject: [PATCH 1/2] 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 Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm --- .changeset/release-debug-logging.md | 10 ++++++++++ packages/host/cpp/Logger.cpp | 7 ++++++- packages/host/cpp/Logger.hpp | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 .changeset/release-debug-logging.md diff --git a/.changeset/release-debug-logging.md b/.changeset/release-debug-logging.md new file mode 100644 index 00000000..218065a6 --- /dev/null +++ b/.changeset/release-debug-logging.md @@ -0,0 +1,10 @@ +--- +"react-native-node-api": patch +--- + +Stop emitting `log_debug`'s per-addon diagnostic chatter (library +found/loaded, symbol resolution, ...) in release builds. It is now compiled +out in `NDEBUG` builds (CMake's `Release`/`MinSizeRel`/`RelWithDebInfo` +configurations, and Xcode's default `Release` configuration), mirroring React +Native's own dev/release logging split. `log_warning` and `log_error` are +unaffected and keep firing in every build type. diff --git a/packages/host/cpp/Logger.cpp b/packages/host/cpp/Logger.cpp index b863fcdf..84f39dff 100644 --- a/packages/host/cpp/Logger.cpp +++ b/packages/host/cpp/Logger.cpp @@ -63,13 +63,18 @@ void log_message_internal(LogLevel level, const char *format, va_list args) { namespace callstack::react_native_node_api { +// See the comment on the declaration in Logger.hpp: in release (NDEBUG) +// builds log_debug is an inline no-op defined in the header, so this +// definition only exists (and only needs to exist) outside of NDEBUG. +#ifndef NDEBUG void log_debug(const char *format, ...) { - // TODO: Disable logging in release builds va_list args; va_start(args, format); log_message_internal(LogLevel::Debug, format, args); va_end(args); } +#endif + void log_warning(const char *format, ...) { va_list args; va_start(args, format); diff --git a/packages/host/cpp/Logger.hpp b/packages/host/cpp/Logger.hpp index c064e7da..da29588d 100644 --- a/packages/host/cpp/Logger.hpp +++ b/packages/host/cpp/Logger.hpp @@ -3,7 +3,33 @@ #include namespace callstack::react_native_node_api { + +// log_debug is verbose, per-addon diagnostic chatter (library found/loaded, +// symbol resolution, ...) emitted on every addon load. It is compiled out +// entirely in release builds: NDEBUG is what CMake's Release/MinSizeRel/ +// RelWithDebInfo configurations define (and what Xcode's Release +// configuration defines by default), mirroring the dev/release split React +// Native itself draws with `__DEV__`. Making the release build's log_debug an +// inline no-op - rather than compiling Logger.cpp's definition and letting it +// run - drops both the log line and its argument-formatting cost from the +// startup path, and lets the optimizer elide side-effect-free call sites +// entirely. +// +// This is compile-time only: there is currently no runtime override to +// re-enable it in a shipped release build to chase a release-only bug (there +// is no existing mechanism in this codebase - env var, JS-settable flag, etc. +// - to hook one into without adding new plumbing of its own). Build a +// Debug or RelWithDebInfo artifact to get this output back; revisit with a +// runtime toggle if that turns out to be too painful in practice. +#ifdef NDEBUG +inline void log_debug(const char *, ...) {} +#else void log_debug(const char *format, ...); +#endif + +// log_warning/log_error keep firing unconditionally in every build type, +// including release (NDEBUG) ones. void log_warning(const char *format, ...); void log_error(const char *format, ...); + } // namespace callstack::react_native_node_api From a7f19136c060516d5c3f522e07dd4a0bc3d3ee22 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 10:56:35 +0000 Subject: [PATCH 2/2] 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 Claude-Session: https://claude.ai/code/session_01NmbKZsRagnasxGVXtnCLoF --- .claude/CLAUDE.md | 27 +++++++++++++++++++++++++++ packages/host/cpp/Logger.cpp | 3 --- packages/host/cpp/Logger.hpp | 21 ++------------------- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 173babe3..31fda3c4 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -4,6 +4,33 @@ Guidance specific to Claude Code sessions that open pull requests against this repository (including automated/scheduled sessions). See the root `CLAUDE.md` and `AGENTS.md` for everything else. +## Keep inline comments very brief — put the color in the PR description + +Default to **no comment at all**. Write one only when it carries knowledge a +reader cannot get from the code itself plus a `git blame` pointing at the PR +that introduced it. When you do write one, keep it to a line or two. + +Rationale, rejected alternatives, benchmark numbers, "we tried X and it +didn't work", links to upstream issues, and anything that reads as a history +lesson belong in the **PR description** (and, where user-facing, the +changeset) — not in the source. Those places are where a reader who has +already found the line via `git blame` will end up anyway, and they don't +have to be maintained as the code around them changes. + +Concretely, do not write comments that: + +- restate what the next line already says; +- explain why an alternative implementation was _not_ chosen; +- narrate the change (`// now compiled out in release builds`) — that is a + commit message, and it goes stale the moment the code moves; +- document a well-known toolchain fact (e.g. what `NDEBUG` means) that a + reader can look up. + +Comments that _do_ earn their place: a non-obvious constraint the compiler or +platform imposes, a workaround with the exact condition that makes it +removable (see the upstream-fix guidance in `AGENTS.md`), or a subtle +invariant a future edit could silently break. + ## Attach CI labels when you open a PR `.github/workflows/check.yml`'s `pull_request` trigger only fires on diff --git a/packages/host/cpp/Logger.cpp b/packages/host/cpp/Logger.cpp index 84f39dff..ce9d7c0c 100644 --- a/packages/host/cpp/Logger.cpp +++ b/packages/host/cpp/Logger.cpp @@ -63,9 +63,6 @@ void log_message_internal(LogLevel level, const char *format, va_list args) { namespace callstack::react_native_node_api { -// See the comment on the declaration in Logger.hpp: in release (NDEBUG) -// builds log_debug is an inline no-op defined in the header, so this -// definition only exists (and only needs to exist) outside of NDEBUG. #ifndef NDEBUG void log_debug(const char *format, ...) { va_list args; diff --git a/packages/host/cpp/Logger.hpp b/packages/host/cpp/Logger.hpp index da29588d..7bec0047 100644 --- a/packages/host/cpp/Logger.hpp +++ b/packages/host/cpp/Logger.hpp @@ -4,31 +4,14 @@ namespace callstack::react_native_node_api { -// log_debug is verbose, per-addon diagnostic chatter (library found/loaded, -// symbol resolution, ...) emitted on every addon load. It is compiled out -// entirely in release builds: NDEBUG is what CMake's Release/MinSizeRel/ -// RelWithDebInfo configurations define (and what Xcode's Release -// configuration defines by default), mirroring the dev/release split React -// Native itself draws with `__DEV__`. Making the release build's log_debug an -// inline no-op - rather than compiling Logger.cpp's definition and letting it -// run - drops both the log line and its argument-formatting cost from the -// startup path, and lets the optimizer elide side-effect-free call sites -// entirely. -// -// This is compile-time only: there is currently no runtime override to -// re-enable it in a shipped release build to chase a release-only bug (there -// is no existing mechanism in this codebase - env var, JS-settable flag, etc. -// - to hook one into without adding new plumbing of its own). Build a -// Debug or RelWithDebInfo artifact to get this output back; revisit with a -// runtime toggle if that turns out to be too painful in practice. +// Inline (rather than a no-op in Logger.cpp) to let the optimizer drop the +// argument evaluation at every call site. #ifdef NDEBUG inline void log_debug(const char *, ...) {} #else void log_debug(const char *format, ...); #endif -// log_warning/log_error keep firing unconditionally in every build type, -// including release (NDEBUG) ones. void log_warning(const char *format, ...); void log_error(const char *format, ...);