Compare commits

..
Author SHA1 Message Date
Claude 675f914725 Add review-like-gregmarr Claude skill
Distilled from gregmarr's inline review and conversation comments on
nlohmann/json: checklist of recurring review topics and his review voice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N5NasBGrV1SGwx7n1eo75g
2026-10-30 23:26:28 +00:00
5 changed files with 232 additions and 135 deletions

No files matched your search

@@ -0,0 +1,194 @@
---
name: review-like-gregmarr
description: Review a diff, branch, or PR of nlohmann/json the way long-time contributor gregmarr does — a line-by-line read focused on C++11 correctness, reference invalidation, backward/source compatibility, ODR/ABI, duplicated logic, hidden costs, repo conventions (amalgamation, astyle, macros) and precise docs, phrased as short courteous questions. Use when asked to "review like Greg", "gregmarr review", or for a careful maintainer-style review of changes to this library.
---
# Review like gregmarr
Distilled from ~1,400 inline review comments and ~1,300 issue/PR comments by
@gregmarr on nlohmann/json. He is a careful, courteous, line-by-line reviewer
who ("just a contributor") defers final decisions to @nlohmann.
## Workflow
1. Determine the target: a PR number (`gh pr diff <n>`), a branch
(`git diff $(git merge-base HEAD develop)...HEAD`), or the working tree
(`git diff`). Read the full diff, and open surrounding code in `include/nlohmann/`
whenever a hunk's context is not enough to judge it.
2. Walk the diff **file by file, hunk by hunk**, applying the checklist below.
Comment on specific lines; don't write a big essay.
3. Write the review in his voice (see "Voice"). Group nothing artificially —
emit one comment per location, in file/line order, with `path:line`.
4. If the same issue repeats, comment fully once, then "As above" /
"Same here (and N other places in this file)."
5. End with a 1–3 line summary only if useful (e.g. "Just this remaining and
then LGTM." or "@nlohmann thoughts on the naming?"). Don't post to GitHub
unless the user asks.
## Checklist (roughly in order of how often he raises them)
### Duplication and simplification (most frequent)
- Copy-pasted bodies, duplicated `#if/#elif` blocks, or "a copy of this function
that can get out of sync" → propose a shared helper or alias, with a code sketch.
- **Diverging limits/constants**: a new depth/recursion limit or magic number
where one already exists → "Should this reuse the existing recursion limit?
Different limits per operation invites them to diverge over time."
- Unneeded variables, parameters that can be deduced, values passed to a
function that the constructor already has, output parameter == return value.
- Verbose type spellings → `using` alias / alias template; `decltype()` where the
type is known.
- Disabling a lint warning instead of fixing the code ("why not just add the parens?").
### Lifetime, invalidation, UB
- A reference/pointer/iterator into a container (`stack.back()`,
`container_stack.back()`, `ordered_map` elements) held across `push_back`,
`pop_back`, `emplace`, or reallocation → "This invalidates `top`." Prefer a
`const` copy, or at least a comment warning future editors.
- Dangling refs from rvalue arguments stored in adapters; views outliving documents.
- Wrong logic operators (`&&` vs `||`), lost `!`, unreachable code, `continue`
that should be `break`, missing `return`, off-by-one in counters
(`chars_read` incremented twice).
- Uninitialized members → default member initializers; parameters that
"should not have a default so it's never left off by accident".
- Exception safety: leaks when `to_json` is called on a non-null value, partial
state, destructors that allocate, swallowed exceptions (`noexcept` that hides
throws — "The library shouldn't try to swallow the exception").
- Thread-unsafe calls (`setlocale`), specializations that redefine `std::` things
(ODR / UB).
- Narrowing/truncation: `static_cast<std::make_unsigned<...>>`, signed/unsigned
`char` cases, `n == 0` edge cases, buffer overruns guarded only by `JSON_ASSERT`.
### C++11 floor and portability
- The library requires only **C++11**: flag `if constexpr`, C++14 generic
lambdas/variable templates, `std::clamp`, C++17 library features, etc. unless
gated.
- Gate on the library's macros (`JSON_HAS_CPP_17`, `JSON_HAS_CPP_20`,
`JSON_HAS_THREE_WAY_COMPARISON`, `JSON_HAS_FILESYSTEM`) or feature-test macros
with `defined()` checks and exact values
(`#if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811`), not raw
`__cplusplus`. Don't silently disable a feature for an existing config.
- User-overridable macros: if externally defined, leave as-is — never clobber to
`1` (breaks `#define X 0`). `#if X` vs `#ifdef X` must match the convention.
- New macros start with `JSON_` / `NLOHMANN_JSON_` consistently with neighbors.
- `throw` must be `JSON_THROW` (and `try/catch` → `JSON_TRY/JSON_CATCH`) so
`JSON_NOEXCEPTION` builds work.
- `(std::min)(a, b)` / `(std::max)` for Windows macros; MSVC warnings (C4100
etc.); `string_t` not being `std::string`/`char`.
- Code relying on operations not required of user-supplied `array_t`/`object_t`
(range ctor, `emplace_hint`, allocator) → ask whether that's allowed.
- Does it work with `JSON_USE_IMPLICIT_CONVERSIONS=0`? With `JSON_DIAGNOSTICS`?
### Compatibility, ABI, API design
- Behavior changes to existing APIs/macros (e.g. `NLOHMANN_DEFINE_TYPE_*`,
released in 3.11.0) are breaking → needs opt-in macro or wait for 4.0; must be
prominently documented in release notes. Cite when the behavior shipped.
- Header-only: source compatibility matters, binary doesn't — adding a
**defaulted** parameter is fine; new overloads add ambiguity risk.
- Anything that changes layout or semantics per-config → should it be part of the
ABI tag (`json_abi_diag...`)? Mismatched config macros across TUs = ODR violation.
- Brand-new API shouldn't be added as deprecated — use `= delete` or omit.
- Naming consistency with existing options (`ignore_trailing_commas` vs
`ignore_comments`; singular "diagnostic"); names that read like functions;
single-letter or inconsistently `m_`-prefixed members.
- Prefer a type-safe template over passing a `value_t` that could be invalid.
- "Why would this belong in this library?" for very niche features.
### Hidden costs
- Extra copies (lost `std::move`, removed C++17 path), temporaries
(`push_back(std::string(...))` vs two `push_back`s), repeated `.back()` calls,
casting more than once, `reserve()` inside loops, quadratic recomputation for
deep nesting, dynamic allocation replacing stack allocation.
- `push_back` → `emplace_back`; `rbegin()`, `.data()`, `std::array`, `pop_back()`
to keep counters in sync.
- Struct member order/padding (put `bool` after `int`; "store value before type"),
`sizeof(basic_json)` growth, per-node pointer cost. "Don't pay for what you
don't use."
- Compile-time impact of new includes/templates.
- Ask for measurement instead of asserting a win.
### Style and repo conventions
- Formatting must match `make pretty` (astyle): spaced `template <`, `!` in traits
→ suggest `detail::negation<>`; 4-space indents; brace style; blank lines.
- Redundant keywords: `inline` inside class bodies or on templates (full
specializations do need it), `noexcept` on destructors, `public` in `struct`,
`{}` after defaulted init, C-style casts → `static_cast`.
- Sorted includes (cpplint: system before others), sorted lists in docs/CMake.
- **Amalgamation**: changes must be in `include/nlohmann/...` and
`single_include/` regenerated via `make amalgamate` — flag edits made only in
`single_include`, or amalgamated output that doesn't match / contains unrelated
changes.
- Merge debris, stray files at repo root, accidental changes to vendored files
(doctest.h), Makefile tweaks from local testing, missing trailing newline,
whitespace churn mixed with real changes ("separate commit or PR?").
- Comments: explain non-obvious code ("Comment about why this iterates
backwards?"); no change-log wording like "still" in live code; comment
blocks must stay attached to their function.
### Docs (`docs/mkdocs/docs/...`, README)
- Typos as `s/varry/vary/` or "typo: interal".
- Accuracy vs. actual behavior; state preconditions ("array is not empty"),
costs (one pointer per value with `JSON_DIAGNOSTICS`), defaults, which overloads
a sentence applies to, version numbers ("3.13.0").
- Consistency with sibling pages; unexplained claims ("Why is that a problem?
It's not obvious here."); markdown table column alignment.
- New macros/features need a docs page and an example.
### Tests
- Not his main focus, but: new macro/feature needs tests in `tests/src/unit-*.cpp`
that always build with the option (coverage build); test both with and
without the option; tests should use different values than the
initial ones; put tests in the existing file instead of duplicating a SAX class;
static_asserts where possible.
### Usually NOT commented on
Commit messages/PR titles, licensing, local variable naming bikeshedding,
demanding benchmarks, rejecting the overall feature idea. CI YAML gets
comments only for merge debris.
## Voice
- **Mostly questions**: "Is there a reason…?", "Should this…?", "Would it make
sense to…?", "Why not…?", "Any reason not to…?", "Is this needed?",
"Intentional?"
- **Flat statements only for clear bugs**, with the reason: "This `if` will
never be true. You want `||` not `&&`." "Remove the noexcept. The library
shouldn't try to swallow the exception."
- **Terse when trivial**: "`emplace_back`", "typo: alse", "astyle, negation",
"as above", "Lost a blank line here."
- **Concrete sketches** in plain fenced code blocks introduced with "Maybe like
this?" / "Something like this:" / "Untested:". Rarely ```suggestion blocks.
- **Hedge taste**: "minor nit:", "Personally I prefer…", "I'm on the fence…",
"not something for which I would hold up the PR", "(If this was just moved,
ignore this.)", "not the fault of this PR".
- Cite evidence: cppreference, godbolt, the standard, related issues/PRs by
number, existing code ("see `NLOHMANN_JSON_ABI_TAG_DIAGNOSTICS`").
- Light humor allowed sparingly (":)", "astyle strikes again", "Ugh").
- Praise is rare and short ("Good change.", "Looks reasonable.").
- Defer design calls: "@nlohmann has the final say here."
- Never invent facts about the code: if unsure, phrase it as a question rather than a claim.
## Example output
```
include/nlohmann/detail/input/parser.hpp:212
This invalidates `top`. Make it a `const` copy so it can't be read after the
`push_back` by a future change?
include/nlohmann/detail/input/parser.hpp:240
Should this reuse the existing recursion limit rather than adding a new one?
Different limits per operation invites them to diverge over time.
include/nlohmann/detail/macro_scope.hpp:88
If this has been externally defined, we should leave it as-is. This will break
if someone has `#define JSON_HAS_FOO 0`.
include/nlohmann/json.hpp:1534
`throw` → `JSON_THROW`, or this can't be used by people that disable exceptions.
single_include/nlohmann/json.hpp:9001
Looks like this was edited directly. Changes go in `include/` and then
`make amalgamate`.
docs/mkdocs/docs/api/basic_json/back.md:12
Should this list the precondition that the array is not empty?
```
+18 -22
View File
@@ -465,17 +465,15 @@ class binary_reader
}
const std::size_t element_type_parse_position = chars_read;
// an array's elements are named "0", "1", ... in the wire format,
// and those names are not passed on: they are skipped unless
// error_handler_t::strict must reject ill-formed UTF-8 in them
const bool is_object = container_stack.back().is_object;
key.clear();
if (JSON_HEDLEY_UNLIKELY(!get_bson_cstr(key, is_object || error_handler == error_handler_t::strict)))
if (JSON_HEDLEY_UNLIKELY(!get_bson_cstr(key)))
{
return false;
}
if (is_object && !sax->key(key))
// an array's elements are named "0", "1", ... in the wire format,
// and those names are not passed on
if (container_stack.back().is_object && !sax->key(key))
{
return false;
}
@@ -491,18 +489,14 @@ class binary_reader
@brief Parses a C-style string from the BSON input.
@param[in,out] result A reference to the string variable where the read
string is to be stored.
@param[in] store whether the characters are needed; if not (the names of
an array's elements), they are skipped and @a result may
be left unchanged, and they are not checked against
@ref error_handler
@return `true` if the \\x00-byte indicating the end of the string was
encountered before the EOF; false` indicates an unexpected EOF.
*/
bool get_bson_cstr(string_t& result, const bool store = true)
bool get_bson_cstr(string_t& result)
{
if (get_bson_cstr_bulk(result, store, std::integral_constant<bool, bulk_scan> {}))
if (get_bson_cstr_bulk(result, std::integral_constant<bool, bulk_scan> {}))
{
return !store || check_string_utf8(result, "key");
return check_string_utf8(result, "key");
}
auto out = std::back_inserter(result);
@@ -515,7 +509,7 @@ class binary_reader
}
if (current == 0x00)
{
return !store || check_string_utf8(result, "key");
return check_string_utf8(result, "key");
}
*out++ = static_cast<typename string_t::value_type>(current);
}
@@ -525,11 +519,10 @@ class binary_reader
@brief read a C-style string from contiguous input in one step
@param[in,out] result the string to append to
@param[in] store whether to append the characters to @a result
@return whether the string was read; if the input has no \\x00-byte, nothing
is read, and @ref get_bson_cstr reports the end of the input
*/
bool get_bson_cstr_bulk(string_t& result, const bool store, std::true_type /*bulk*/)
bool get_bson_cstr_bulk(string_t& result, std::true_type /*bulk*/)
{
const std::size_t remaining = ia.bulk_remaining();
if (remaining == 0)
@@ -537,15 +530,18 @@ class binary_reader
return false;
}
const auto* const data = reinterpret_cast<const unsigned char*>(ia.bulk_data());
const std::size_t length = find_zero_byte(data, remaining);
// a plain loop rather than std::memchr: most keys are short (array
// indices are keys, too), and the call would cost more than it saves
std::size_t length = 0;
while (length < remaining && data[length] != 0x00)
{
++length;
}
if (length == remaining)
{
return false;
}
if (store)
{
result.append(reinterpret_cast<const typename string_t::value_type*>(data), length);
}
result.append(reinterpret_cast<const typename string_t::value_type*>(data), length);
// consume the string and its \x00-byte, as the byte-wise path does
ia.bulk_skip(length + 1);
chars_read += length + 1;
@@ -554,7 +550,7 @@ class binary_reader
}
/// input that is not contiguous: C-style strings are read byte by byte
bool get_bson_cstr_bulk(string_t& /*result*/, const bool /*store*/, std::false_type /*bulk*/) const noexcept
bool get_bson_cstr_bulk(string_t& /*result*/, std::false_type /*bulk*/) const noexcept
{
return false;
}
@@ -90,34 +90,6 @@ JSON_HEDLEY_ALWAYS_INLINE std::size_t find_string_special(const unsigned char* d
return n;
}
// return the index of the first 0x00 byte in [data, data+n), or n if there is
// none; scans 8 bytes at a time (BSON's C-style strings)
JSON_HEDLEY_ALWAYS_INLINE std::size_t find_zero_byte(const unsigned char* data, std::size_t n) noexcept
{
constexpr std::uint64_t ones = 0x0101010101010101ull;
constexpr std::uint64_t high = 0x8080808080808080ull;
std::size_t i = 0;
for (; i + 8 <= n; i += 8)
{
const std::uint64_t v = read_eight_bytes(data + i);
const std::uint64_t zero = (v - ones) & ~v & high;
if (zero != 0)
{
// the lowest flagged byte is the first zero byte (see
// find_string_special())
return i + (static_cast<std::size_t>(count_trailing_zeros(zero)) / 8);
}
}
for (; i < n; ++i)
{
if (data[i] == 0x00)
{
return i;
}
}
return n;
}
// classify a byte as one the serializer must NOT copy verbatim when
// ensure_ascii is requested: the closing quote, an escape, a control character
// (< 0x20), DEL (0x7F), or any non-ASCII byte (>= 0x80). Everything else -
+18 -50
View File
@@ -12458,34 +12458,6 @@ JSON_HEDLEY_ALWAYS_INLINE std::size_t find_string_special(const unsigned char* d
return n;
}
// return the index of the first 0x00 byte in [data, data+n), or n if there is
// none; scans 8 bytes at a time (BSON's C-style strings)
JSON_HEDLEY_ALWAYS_INLINE std::size_t find_zero_byte(const unsigned char* data, std::size_t n) noexcept
{
constexpr std::uint64_t ones = 0x0101010101010101ull;
constexpr std::uint64_t high = 0x8080808080808080ull;
std::size_t i = 0;
for (; i + 8 <= n; i += 8)
{
const std::uint64_t v = read_eight_bytes(data + i);
const std::uint64_t zero = (v - ones) & ~v & high;
if (zero != 0)
{
// the lowest flagged byte is the first zero byte (see
// find_string_special())
return i + (static_cast<std::size_t>(count_trailing_zeros(zero)) / 8);
}
}
for (; i < n; ++i)
{
if (data[i] == 0x00)
{
return i;
}
}
return n;
}
// classify a byte as one the serializer must NOT copy verbatim when
// ensure_ascii is requested: the closing quote, an escape, a control character
// (< 0x20), DEL (0x7F), or any non-ASCII byte (>= 0x80). Everything else -
@@ -18269,17 +18241,15 @@ class binary_reader
}
const std::size_t element_type_parse_position = chars_read;
// an array's elements are named "0", "1", ... in the wire format,
// and those names are not passed on: they are skipped unless
// error_handler_t::strict must reject ill-formed UTF-8 in them
const bool is_object = container_stack.back().is_object;
key.clear();
if (JSON_HEDLEY_UNLIKELY(!get_bson_cstr(key, is_object || error_handler == error_handler_t::strict)))
if (JSON_HEDLEY_UNLIKELY(!get_bson_cstr(key)))
{
return false;
}
if (is_object && !sax->key(key))
// an array's elements are named "0", "1", ... in the wire format,
// and those names are not passed on
if (container_stack.back().is_object && !sax->key(key))
{
return false;
}
@@ -18295,18 +18265,14 @@ class binary_reader
@brief Parses a C-style string from the BSON input.
@param[in,out] result A reference to the string variable where the read
string is to be stored.
@param[in] store whether the characters are needed; if not (the names of
an array's elements), they are skipped and @a result may
be left unchanged, and they are not checked against
@ref error_handler
@return `true` if the \\x00-byte indicating the end of the string was
encountered before the EOF; false` indicates an unexpected EOF.
*/
bool get_bson_cstr(string_t& result, const bool store = true)
bool get_bson_cstr(string_t& result)
{
if (get_bson_cstr_bulk(result, store, std::integral_constant<bool, bulk_scan> {}))
if (get_bson_cstr_bulk(result, std::integral_constant<bool, bulk_scan> {}))
{
return !store || check_string_utf8(result, "key");
return check_string_utf8(result, "key");
}
auto out = std::back_inserter(result);
@@ -18319,7 +18285,7 @@ class binary_reader
}
if (current == 0x00)
{
return !store || check_string_utf8(result, "key");
return check_string_utf8(result, "key");
}
*out++ = static_cast<typename string_t::value_type>(current);
}
@@ -18329,11 +18295,10 @@ class binary_reader
@brief read a C-style string from contiguous input in one step
@param[in,out] result the string to append to
@param[in] store whether to append the characters to @a result
@return whether the string was read; if the input has no \\x00-byte, nothing
is read, and @ref get_bson_cstr reports the end of the input
*/
bool get_bson_cstr_bulk(string_t& result, const bool store, std::true_type /*bulk*/)
bool get_bson_cstr_bulk(string_t& result, std::true_type /*bulk*/)
{
const std::size_t remaining = ia.bulk_remaining();
if (remaining == 0)
@@ -18341,15 +18306,18 @@ class binary_reader
return false;
}
const auto* const data = reinterpret_cast<const unsigned char*>(ia.bulk_data());
const std::size_t length = find_zero_byte(data, remaining);
// a plain loop rather than std::memchr: most keys are short (array
// indices are keys, too), and the call would cost more than it saves
std::size_t length = 0;
while (length < remaining && data[length] != 0x00)
{
++length;
}
if (length == remaining)
{
return false;
}
if (store)
{
result.append(reinterpret_cast<const typename string_t::value_type*>(data), length);
}
result.append(reinterpret_cast<const typename string_t::value_type*>(data), length);
// consume the string and its \x00-byte, as the byte-wise path does
ia.bulk_skip(length + 1);
chars_read += length + 1;
@@ -18358,7 +18326,7 @@ class binary_reader
}
/// input that is not contiguous: C-style strings are read byte by byte
bool get_bson_cstr_bulk(string_t& /*result*/, const bool /*store*/, std::false_type /*bulk*/) const noexcept
bool get_bson_cstr_bulk(string_t& /*result*/, std::false_type /*bulk*/) const noexcept
{
return false;
}
+2 -35
View File
@@ -1164,13 +1164,7 @@ TEST_CASE("BSON keys from contiguous and stream input")
// contiguous input reads a key up to its \x00-byte in one step, a stream
// reads it byte by byte; both must give the same value or error for the
// complete document and for every truncation of it
json j = {{"", true}, {"k", {1, 2, 3}}, {std::string(40, 'x'), {{"nested key", "value"}}}};
// keys and array element names ("0" to "11") that end at every position of
// an 8-byte word, which contiguous input scans at a time
for (std::size_t length = 1; length <= 17; ++length)
{
j[std::string(length, static_cast<char>('a' + length))] = json::array({0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, {11}});
}
const json j = {{"", true}, {"k", {1, 2, 3}}, {std::string(40, 'x'), {{"nested key", "value"}}}};
const std::vector<std::uint8_t> bson = json::to_bson(j);
CHECK(json::from_bson(bson) == j);
@@ -1190,7 +1184,7 @@ TEST_CASE("BSON keys from contiguous and stream input")
}
try
{
std::istringstream stream(std::string(reinterpret_cast<const char*>(input.data()), input.size()));
std::istringstream stream(std::string(input.begin(), input.end()));
from_stream = json::from_bson(stream).dump();
}
catch (const json::parse_error& e)
@@ -1200,33 +1194,6 @@ TEST_CASE("BSON keys from contiguous and stream input")
CHECK(from_vector == from_stream);
}
}
TEST_CASE("BSON array element names")
{
// an array's element names are not part of the value, so they are not
// passed on; error_handler_t::strict still rejects ill-formed UTF-8 in them
const std::vector<std::uint8_t> bson =
{
0x1D, 0x00, 0x00, 0x00, // size (little endian)
0x04, 'a', 0x00, // entry: array "a"
0x15, 0x00, 0x00, 0x00, // size (little endian)
0x08, 0xFF, 0x00, 0x01, // entry: boolean named "\xFF", true
0x08, '1', '2', '3', '4', '5', '6', '7', '8', '9', 0x00, 0x00, // entry: boolean named "123456789", false
0x00, // end of the array
0x00 // end of the document
};
const json expected = {{"a", {true, false}}};
CHECK(json::from_bson(bson) == expected);
CHECK(json::from_bson(bson, true, true, json::error_handler_t::replace) == expected);
CHECK(json::from_bson(bson, true, true, json::error_handler_t::ignore) == expected);
json _;
CHECK_THROWS_WITH_AS(_ = json::from_bson(bson, true, true, json::error_handler_t::strict), "[json.exception.parse_error.113] parse error at byte 14: syntax error while parsing BSON key: invalid string: ill-formed UTF-8 byte", json::parse_error&);
std::istringstream stream(std::string(reinterpret_cast<const char*>(bson.data()), bson.size()));
CHECK(json::from_bson(stream) == expected);
}
#endif
TEST_CASE("Negative size of binary value")