Skip to content

Commit a7f1913

Browse files
committed
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
1 parent 7fcd7c3 commit a7f1913

3 files changed

Lines changed: 29 additions & 22 deletions

File tree

‎.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: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,6 @@ void log_message_internal(LogLevel level, const char *format, va_list args) {
6363

6464
namespace callstack::react_native_node_api {
6565

66-
// See the comment on the declaration in Logger.hpp: in release (NDEBUG)
67-
// builds log_debug is an inline no-op defined in the header, so this
68-
// definition only exists (and only needs to exist) outside of NDEBUG.
6966
#ifndef NDEBUG
7067
void log_debug(const char *format, ...) {
7168
va_list args;

‎packages/host/cpp/Logger.hpp‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,31 +4,14 @@
44

55
namespace callstack::react_native_node_api {
66

7-
// log_debug is verbose, per-addon diagnostic chatter (library found/loaded,
8-
// symbol resolution, ...) emitted on every addon load. It is compiled out
9-
// entirely in release builds: NDEBUG is what CMake's Release/MinSizeRel/
10-
// RelWithDebInfo configurations define (and what Xcode's Release
11-
// configuration defines by default), mirroring the dev/release split React
12-
// Native itself draws with `__DEV__`. Making the release build's log_debug an
13-
// inline no-op - rather than compiling Logger.cpp's definition and letting it
14-
// run - drops both the log line and its argument-formatting cost from the
15-
// startup path, and lets the optimizer elide side-effect-free call sites
16-
// entirely.
17-
//
18-
// This is compile-time only: there is currently no runtime override to
19-
// re-enable it in a shipped release build to chase a release-only bug (there
20-
// is no existing mechanism in this codebase - env var, JS-settable flag, etc.
21-
// - to hook one into without adding new plumbing of its own). Build a
22-
// Debug or RelWithDebInfo artifact to get this output back; revisit with a
23-
// runtime toggle if that turns out to be too painful in practice.
7+
// Inline (rather than a no-op in Logger.cpp) to let the optimizer drop the
8+
// argument evaluation at every call site.
249
#ifdef NDEBUG
2510
inline void log_debug(const char *, ...) {}
2611
#else
2712
void log_debug(const char *format, ...);
2813
#endif
2914

30-
// log_warning/log_error keep firing unconditionally in every build type,
31-
// including release (NDEBUG) ones.
3215
void log_warning(const char *format, ...);
3316
void log_error(const char *format, ...);
3417

0 commit comments

Comments
 (0)