diff --git a/dirtiles.cpp b/dirtiles.cpp index f930f2ad..b7dd3945 100644 --- a/dirtiles.cpp +++ b/dirtiles.cpp @@ -261,12 +261,9 @@ sqlite3 *dirmeta2tmp(const char *fname) { } for (const auto &e : o->entries()) { - // Skip, rather than just warn about, anything that isn't a - // string/string pair: reading a non-string through string() - // would assert in a debug build and misinterpret the node's - // storage in a release build. (A metadata.json written by - // something other than tippecanoe may well have numeric - // minzoom/maxzoom or a nested "json" object.) + // Skip, not just warn: a metadata.json from another tool may + // have a numeric minzoom or a nested object, and string() + // asserts on the type. if (e.key->type != JSON_STRING || e.value->type != JSON_STRING) { fprintf(stderr, "%s: non-string in metadata\n", name.c_str()); continue; diff --git a/geojson.cpp b/geojson.cpp index b445e299..d9bc569f 100644 --- a/geojson.cpp +++ b/geojson.cpp @@ -238,9 +238,7 @@ struct json_serialize_action : json_feature_action { std::string layername; int add_feature(json_object *geometry, bool geometrycollection, json_object *properties, json_object *id, json_object *tippecanoe, json_object *feature) { - // This only ever receives json_read results from geojson-loop, whose - // parser is still attached. json_read_tree / json_disconnect clear - // every parser pointer, so a detached tree would null-deref here. + // Only json_read results reach this; a detached tree has no parser. assert(geometry->parser != nullptr); sst->line = geometry->parser->line; if (geometrycollection) { diff --git a/jsonpull/jsonpull.cpp b/jsonpull/jsonpull.cpp index f3e76e8f..c5aa5027 100644 --- a/jsonpull/jsonpull.cpp +++ b/jsonpull/jsonpull.cpp @@ -88,13 +88,8 @@ static inline int read_wrap(json_pull *j) { return c; } -// Construct an instance of the right subclass for the given type. // JSON_TRUE / JSON_FALSE / JSON_NULL and the parse-token types are bare // json_objects; the value-bearing types each get their own subclass. -// -// Returns a json_object_ptr (unique_ptr with a type-dispatching deleter, -// see jsonpull.h), so the caller doesn't have to remember which subclass -// was constructed when it eventually deletes. static json_object_ptr make_object(json_type type, json_object *parent, json_pull *jp) { switch (type) { case JSON_NUMBER: @@ -114,11 +109,8 @@ static inline json_pull::parse_frame *current_frame(json_pull *j) { return j->container_stack.empty() ? nullptr : &j->container_stack.back(); } -// Construct a new node of `type` and install it as a child of the -// current container (or as the parser's root, if the container stack -// is empty). Returns a borrowed pointer into the parser-owned tree; -// the unique_ptr that owns the node lives in whichever vector slot -// we just pushed it into. Returns nullptr on error after setting +// Install a new node of `type` in the current container, or as the parser's +// root if the stack is empty. Returns it borrowed, or nullptr after setting // j->error. static json_object *add_object(json_pull *j, json_type type) { json_pull::parse_frame *f = current_frame(j); @@ -137,9 +129,8 @@ static json_object *add_object(json_pull *j, json_type type) { } } else if (c->type == JSON_HASH) { if (f->expect == JSON_VALUE) { - // JSON_VALUE is only set by a colon, a colon requires - // JSON_COLON, and only pushing a key sets that, so there - // is always an entry waiting for its value here. + // A colon is the only thing that sets JSON_VALUE, and it + // requires a key already pushed. assert(!c->entries().empty()); c->entries().back().value = std::move(o); f->expect = JSON_COMMA; @@ -237,8 +228,6 @@ again: if (o == nullptr) { return nullptr; } - // add_object already installed `o` in the parent (or the - // parser's root) as a unique_ptr; the frame just borrows. j->container_stack.push_back({o, JSON_ITEM}); if (cb != nullptr) { @@ -268,8 +257,6 @@ again: } } - // Pop the frame; ownership of `cc` stays with whatever - // surrounding container (or jp->root) installed it. j->container_stack.pop_back(); return cc; } @@ -520,9 +507,7 @@ again: /////////////////////////// Strings case '"': { - // Reuse the parser-wide string buffer so we don't construct a - // fresh std::string (with its inevitable SSO->heap promotion - // and capacity doublings) for every JSON_STRING token. + // Reused across tokens; see json_pull::string_buffer. std::string &val = j->string_buffer; val.clear(); @@ -648,11 +633,7 @@ again: json_object *s = add_object(j, JSON_STRING); if (s != nullptr) { - // Copy (don't move) so j->string_buffer retains its - // grown capacity for the next token. The copy is a - // single right-sized allocation plus one memcpy, which - // is cheaper than the multiple capacity doublings the - // per-token std::string would otherwise incur. + // Copy, not move, so the buffer keeps its capacity. s->string() = val; } return s; @@ -667,10 +648,6 @@ json_object *json_read(json_pull_ptr &j) { return json_read_separators(j, nullptr, nullptr); } -// Forward declaration so json_read_tree can sever the tree it hands out -// from the parser -- this lets callers (like the filter loaders) keep the -// returned tree past the parser's lifetime without having to follow up -// with a separate json_disconnect call. static void detach_subtree(json_object *o); json_object_ptr json_read_tree(json_pull_ptr &p) { @@ -678,10 +655,6 @@ json_object_ptr json_read_tree(json_pull_ptr &p) { while ((j = json_read(p)) != nullptr) { if (j->parent == nullptr) { - // The parser owns the top-level value via p->root; - // transfer ownership out to the caller and detach - // the subtree from the parser so the caller can - // outlive the json_pull. json_object_ptr tree = std::move(p->root); detach_subtree(tree.get()); return tree; @@ -691,20 +664,13 @@ json_object_ptr json_read_tree(json_pull_ptr &p) { return nullptr; } -// Take ownership of `o` away from its parent (or from the parser's -// root) by moving the owning json_object_ptr out of whatever vector -// slot or hash entry holds it. Returns the unique_ptr to the caller, -// who is now solely responsible for it. Returns an empty -// json_object_ptr if `o` is not currently owned by a parent or by -// the parser (e.g. already detached, or only borrowed from somewhere -// untracked). +// Move the owning json_object_ptr out of whatever vector slot or hash entry +// holds `o`. Empty if nothing tracked owns it -- already detached, or borrowed +// from elsewhere. // -// For a hash, removing a single key or value individually would -// disturb the surrounding key/value pairing, so we replace the -// extracted half with a fresh JSON_NULL placeholder and only erase -// the entry once both halves have been detached. This matches the -// historical json_disconnect semantics for partially-disconnected -// pairs. +// Detaching one half of a hash entry would disturb the surrounding key/value +// pairing, so the extracted half is replaced by a JSON_NULL placeholder and the +// entry is erased only once both halves are gone. static json_object_ptr take_from_owner(json_object *o) { if (o == nullptr) { return nullptr; @@ -756,37 +722,13 @@ static json_object_ptr take_from_owner(json_object *o) { return nullptr; } -// json_free splices `o` out of its parent (if any), or out of the -// parser's root (if `o` is the most recently completed top-level -// value), and destroys the subtree. After this call, `o` is a -// dangling pointer and must not be used. -// -// geojson-loop.cpp relies on this to release each feature after it -// has been serialized, so that already-serialized features don't sit -// in memory while subsequent features are parsed. -// -// Unlike json_disconnect, this does NOT walk the subtree clearing -// parent/parser back-pointers, because the subtree is about to be -// destroyed and those pointers will never be observed again -- the -// unique_ptr returned by take_from_owner goes out of scope at the end -// of this function and runs the type-dispatching deleter. +// Splice `o` out of its owner and destroy it; `o` dangles afterwards. No need +// to clear back-pointers, since nothing will observe them again. void json_free(json_object *o) { (void) take_from_owner(o); } -// Walk the subtree clearing the parser back-pointers, so the detached -// subtree can outlive the json_pull it was parsed from. Every `parser` -// pointer has to go: the json_pull may be destroyed while the subtree -// lives on, and a stale one would dangle. -// -// The `parent` pointers *inside* the subtree are deliberately left alone. -// They are non-owning raw pointers, so keeping them cannot create a -// reference cycle or hold anything alive, and they point at nodes the -// caller now owns as one unit -- they stay valid for exactly as long as -// the subtree does. Keeping them also means the tree stays navigable -// upwards, and that json_free() / json_disconnect() keep working on -// interior nodes of a detached tree, both of which need o->parent to -// find the node's owner. +// See Ownership model in jsonpull.h for why only `parser` is cleared here. static void clear_parser_pointers(json_object *o) { if (o == nullptr) { return; @@ -807,11 +749,9 @@ static void clear_parser_pointers(json_object *o) { o->parser = nullptr; } -// Sever a subtree that take_from_owner() has just moved out of the tree it -// belonged to. The root's own `parent` is the one back-pointer that must be -// cleared: it pointed *out* of the subtree, at a node the parser still owns -// and may destroy, and leaving it set would make a later json_free() on this -// root hunt for itself in a container that no longer holds it. +// The root's `parent` pointed out of the subtree, at a node the parser still +// owns; leaving it set would make a later json_free look for this node in a +// container that no longer holds it. static void detach_subtree(json_object *o) { if (o == nullptr) { return; @@ -835,10 +775,8 @@ static void json_print_one(std::string &val, const json_object *o) { } else if (o->type == JSON_STRING) { val.push_back('\"'); - // Range over the string rather than walking c_str(): the value is a - // std::string now and may legitimately contain an embedded NUL, which - // the control-character branch below escapes as a \u sequence like any - // other control character. + // Range, not c_str(): the value may contain an embedded NUL, which the + // control-character branch below escapes like any other. for (char c : o->string()) { if (c == '\\' || c == '"') { val.push_back('\\'); diff --git a/jsonpull/jsonpull.h b/jsonpull/jsonpull.h index 3ed15afb..5bb7ca63 100644 --- a/jsonpull/jsonpull.h +++ b/jsonpull/jsonpull.h @@ -31,70 +31,42 @@ typedef enum json_type { struct json_object; struct json_pull; -// json_object is non-virtual so that JSON_TRUE / JSON_FALSE / JSON_NULL -// nodes don't have to pay for a vptr, but the typed subclasses -// (json_number, json_string, json_array, json_hash) have non-trivial -// destructors that need to run to free their std::vector / std::string -// members. So json_object_ptr is given a custom empty deleter that -// dispatches on `type` and static_casts to the right subclass before -// `delete`. The deleter is stateless, so the unique_ptr stays one -// pointer wide. +// Ownership model +// +// Every node has exactly one owner: its parent (a json_object_ptr in the +// parent's vector or hash entry), the parser (jp->root) for a top-level value, +// or the caller once json_read_tree / json_disconnect hands the tree over. +// json_read and json_hash_get return borrowed pointers, valid while the owning +// container is intact. +// +// `parent` and `parser` are non-owning, so they cannot form a cycle. Detaching +// clears every `parser` in the subtree, since the json_pull may die first, and +// clears `parent` only on the detached root, which pointed out of the subtree. +// Interior `parent` links stay, so a detached tree is still walkable upwards +// and json_free / json_disconnect still work inside it -- both find a node's +// owner through `parent`. +// +// json_object has no vptr; json_object_deleter switches on `type` and +// static_casts so the right subclass destructor runs. Payloads live in those +// subclasses rather than one wide struct, and the accessors assert on `type` +// before downcasting. +// +// json_pull_ptr is a shared_ptr: a parser is created and freed once. + +// Stateless, so json_object_ptr stays one pointer wide. See Ownership model. struct json_object_deleter { void operator()(json_object *p) const noexcept; }; -// Ownership of a JSON subtree is unique: every node has a single owner, -// which is either its parent (via a json_object_ptr in the parent's -// vector or hash entry) or, for the root, the parser (via jp->root) or -// the caller (after json_read_tree / json_disconnect). -// -// Callers receive borrowed `json_object *` views from json_read, -// json_hash_get, etc.; those pointers stay valid as long as the owning -// container is intact (which, for json_read results, means "until the -// next json_read, json_free, or json_disconnect call on that subtree"). -// -// json_pull_ptr stays a shared_ptr because the parser is created once -// and freed once and the cost of shared_ptr there is irrelevant. typedef std::unique_ptr json_object_ptr; typedef std::shared_ptr json_pull_ptr; -// A single key/value pair inside a JSON_HASH. The pairs are stored in -// insertion order in a single std::vector on json_hash, so -// callers can range-for over `o->entries()` with structured bindings -// (`for (auto &[k, v] : o->entries()) ...`) while still preserving the -// order keys appeared in the source document. +// One key/value pair in a JSON_HASH, held in source order. struct json_entry { json_object_ptr key; json_object_ptr value; }; -// json_object is a small base type that just records the JSON type and -// the back-pointers to its parent and parser. The actual value payload -// lives in a type-specific subclass (json_number, json_string, json_array, -// json_hash), so that JSON_TRUE / JSON_FALSE / JSON_NULL nodes pay only -// the base-class cost and a JSON_HASH does not also drag along a string -// or a number field. Type-tagged accessor methods on the base class -// downcast and return references to the underlying subclass storage. -// -// Children are owned by their parent (via std::vector -// inside json_array / json_hash); the raw `parent` and `parser` -// back-pointers own nothing. json_disconnect() splices a node out of its -// parent and walks the detached subtree clearing every `parser` pointer, -// so the subtree can outlive the original parser. The `parent` pointers -// within the subtree survive -- they refer to nodes the caller now owns -// as one unit -- so a detached tree can still be walked upwards, and -// json_free() / json_disconnect() still work on its interior nodes. Only -// the detached root's `parent`, which pointed out of the subtree, is -// cleared. -// -// json_object intentionally has no virtual functions and no virtual -// destructor; the json_object_ptr deleter (see below in this header) -// switches on `type` and static_casts to the correct subclass before -// `delete`, so each subclass's destructor still runs without costing -// a vptr per node. Dispatch on `type` is what the rest of the code -// already does. The accessor methods assert at debug time that the -// type matches before downcasting. - struct json_object { json_object *parent = nullptr; json_pull *parser = nullptr; @@ -108,17 +80,11 @@ struct json_object { : parent(p), parser(pl), type(t) { } - // Type-tagged accessors. Each one asserts that the receiver is of - // the right kind, then downcasts to the storage in the appropriate - // subclass. Inline so the assert and cast disappear at -O. inline std::string &string(); inline const std::string &string() const; - // Numbers are stored in a discriminated union (double / unsigned / - // signed) so a json_number is only 32 bytes instead of 48. The - // large_*() accessors return 0 when the number is not currently - // stored in that representation, matching the prior convention - // where "0" meant "not set, fall through to the next slot". + // large_unsigned() / large_signed() return 0 when the number is not + // held in that representation, the convention callers already expect. inline double number() const; inline unsigned long long large_unsigned() const; inline long long large_signed() const; @@ -170,12 +136,7 @@ struct json_string : json_object { struct json_array : json_object { std::vector array_value; - // Coordinate-heavy GeoJSON dominates the parse workload, and every - // `[x, y]` (or `[x, y, z]`) pair would otherwise force the inner - // vector through 0 -> 1 -> 2 -> 4 growths plus the matching - // shared_ptr copies. Reserving 2 slots up front eliminates those - // reallocations for the common case and adds only a single small - // allocation for larger rings (which still grow geometrically). + // 2 slots: coordinate pairs dominate the parse workload. json_array() : json_object(JSON_ARRAY) { array_value.reserve(2); @@ -189,10 +150,7 @@ struct json_array : json_object { struct json_hash : json_object { std::vector entries_value; - // Most GeoJSON property hashes have a handful of keys (type, id, - // properties, geometry, plus a few attribute fields). Reserving 4 - // slots avoids the 0 -> 1 -> 2 -> 4 growth chain for the typical - // case while only modestly over-allocating for one-key hashes. + // 4 slots: the typical GeoJSON property hash. json_hash() : json_object(JSON_HASH) { entries_value.reserve(4); @@ -276,9 +234,6 @@ inline void json_object_deleter::operator()(json_object *p) const noexcept { if (p == nullptr) { return; } - // Dispatch on the discriminator so the correct subclass destructor - // runs. json_object has no virtual destructor, so a bare `delete p` - // would skip the std::vector / std::string members of the subclass. switch (p->type) { case JSON_NUMBER: delete static_cast(p); @@ -293,9 +248,7 @@ inline void json_object_deleter::operator()(json_object *p) const noexcept { delete static_cast(p); break; default: - // JSON_TRUE / JSON_FALSE / JSON_NULL (and the parse-token - // types, which never appear as owned nodes) are bare - // json_objects with no extra fields. + // JSON_TRUE / JSON_FALSE / JSON_NULL: no extra fields. delete p; break; } @@ -311,33 +264,21 @@ struct json_pull { ssize_t buffer_tail = 0; ssize_t buffer_head = 0; - // Stack of currently-open containers; the top is the innermost - // container being parsed. Each frame also remembers what token is - // expected next (an item, a comma, a key, a colon, or a value). - // The frame's `container` is a borrowed raw pointer; actual - // ownership of the in-progress container lives in either the - // surrounding container's vector (for nested containers) or - // `root` (for the outermost container). + // Currently-open containers, innermost last, each with the token it + // expects next. `container` is borrowed; the owner is the surrounding + // container, or `root` for the outermost. struct parse_frame { json_object *container; json_type expect; }; std::vector container_stack; - // The most recently completed top-level value. The parser owns - // it (as a unique_ptr) until either: the next top-level value - // starts parsing (the old root is destroyed), the caller calls - // json_read_tree (ownership is transferred out), or the caller - // calls json_free / json_disconnect (the parser's reference is - // dropped explicitly). + // Most recently completed top-level value, owned until the next one + // starts parsing or the caller takes it. json_object_ptr root; - // Scratch buffers reused across tokens so we don't reallocate per - // number/string. number_buffer accumulates raw digits before atof(); - // string_buffer accumulates decoded bytes before being copied into - // the final json_string. Both are cleared (capacity preserved) at - // the start of each token, so once they grow to the largest seen - // size they stop reallocating entirely. + // Reused across tokens, cleared but not shrunk, so they stop + // reallocating once grown to the largest token seen. std::string number_buffer; std::string string_buffer; }; @@ -347,32 +288,20 @@ json_pull_ptr json_begin_string(const char *s); json_pull_ptr json_begin(ssize_t (*read)(struct json_pull *, char *buffer, size_t n), void *source); -// json_end is a thin convenience that resets the caller's json_pull_ptr. -// The parser (and any tree it still owns) is freed when the last -// shared_ptr to it is dropped, so calling json_end is optional if the -// json_pull_ptr will go out of scope on its own. +// Resets the caller's pointer. Optional: the parser frees itself when the +// last json_pull_ptr to it goes away. void json_end(json_pull_ptr &p); typedef void (*json_separator_callback)(json_type type, json_pull *j, void *state); -// json_read returns a borrowed pointer to the next completed JSON node -// in the stream. The returned pointer is valid until the next call that -// extends or trims the parser's tree (the next json_read on the same -// parser, a json_free on the same node, or a json_disconnect that -// extracts the node). Returns nullptr at end of input or on error. -// -// For top-level values, ownership stays with the parser (via jp->root); -// for nested values, ownership stays with the enclosing container. +// The next completed node, borrowed. Valid until the next call that extends +// or trims the parser's tree. nullptr at end of input or on error. json_object *json_read(json_pull_ptr &j); json_object *json_read_separators(json_pull_ptr &j, json_separator_callback cb, void *state); -// json_read_tree drains the next top-level value out of the parser -// and hands ownership to the caller. After it returns, jp->root is -// empty, every `parser` back-pointer in the subtree has been cleared -// (as has the root's `parent`), and the caller's json_object_ptr is the -// only thing keeping the tree alive. The returned tree can outlive the -// json_pull it was parsed from, and stays internally navigable: the -// `parent` pointers between its nodes are left intact. +// Drains the next top-level value out of the parser and hands it over. The +// returned tree can outlive the json_pull. See Ownership model for what the +// detach does and does not clear. json_object_ptr json_read_tree(json_pull_ptr &j); // json_free splices `o` out of its parent (if any), or clears the @@ -381,23 +310,13 @@ json_object_ptr json_read_tree(json_pull_ptr &j); // that must not be used. Safe to call with nullptr. void json_free(json_object *o); -// Splice `o` out of its parent's array/object (or out of the parser's -// root), walk the detached subtree clearing every `parser` back-pointer -// (and the root's `parent`, which pointed out of the subtree), and return -// ownership of the subtree to the caller as a json_object_ptr. After this -// returns, the parser no longer references any node in the subtree, and -// the subtree can outlive the original parser. The `parent` pointers -// among the subtree's own nodes are preserved, so the detached tree can -// still be walked upwards and json_free() / json_disconnect() still work -// on its interior nodes. +// Splices `o` out of whatever owns it and hands it over, same detach as +// json_read_tree. See Ownership model. json_object_ptr json_disconnect(json_object *o); -// Look up `s` in the hash `o`. Returns a borrowed pointer; ownership -// stays with the hash. nullptr if `o` is not a hash, or `s` is absent, -// or the hash is still being parsed and the value slot for `s` is not -// yet filled. A JSON `null` value is *not* one of those cases: it comes -// back as a JSON_NULL node. Accepts a json_object_ptr by reference as a -// convenience so callers don't have to write `.get()`. +// Borrowed value for `s`. nullptr if `o` is not a hash, `s` is absent, or the +// hash is mid-parse with that value slot unfilled -- a JSON `null` is none of +// those, and comes back as a JSON_NULL node. json_object *json_hash_get(const json_object_ptr &o, const char *s); json_object *json_hash_get(json_object *o, const char *s); diff --git a/plugin.cpp b/plugin.cpp index 159c38c7..17463c05 100644 --- a/plugin.cpp +++ b/plugin.cpp @@ -147,15 +147,10 @@ serial_feature parse_feature(json_pull_ptr &jp, int z, unsigned x, unsigned y, s serial_feature sf; while (1) { - // json_read returns each token as the parser produces it, including - // intermediate (still incomplete) container nodes. We must NOT free - // these intermediates here: they belong to the larger feature hash - // still being assembled, and freeing them would splice them out of - // the parent and corrupt the in-progress tree. So the `continue` - // paths below all leave `j` alone; `j` is only freed once it is a - // complete Feature hash -- either just before returning it, or at - // the bottom of the loop if its geometry turned out to be empty -- - // or as `jp->root` when the stream ends. + // json_read also returns the intermediate containers of a feature + // still being assembled, so the `continue` paths leave `j` alone; + // freeing one would splice it out of its parent. `j` is only freed + // once it is a complete Feature. json_object *j = json_read(jp); if (j == nullptr) { if (jp->error != nullptr) { diff --git a/pmtiles_file.cpp b/pmtiles_file.cpp index e4cb7947..4d842869 100644 --- a/pmtiles_file.cpp +++ b/pmtiles_file.cpp @@ -416,11 +416,7 @@ sqlite3 *pmtilesmeta2tmp(const char *fname, const char *pmtiles_map) { state.json_write_hash(); for (const auto &e : o->entries()) { - // Establish that the key really is a string before reading it as - // one, rather than after: string() asserts on the type, so the - // check has to come first to be the thing that catches a bad key. - // (The parser rejects non-string hash keys, so this is belt and - // braces, but the ordering is what makes it meaningful.) + // Check before reading, since string() asserts on the type. if (e.key->type != JSON_STRING) { fprintf(stderr, "%s: non-string key in metadata\n", fname); continue; diff --git a/read_json.cpp b/read_json.cpp index 13798ee6..ec0b1cfd 100644 --- a/read_json.cpp +++ b/read_json.cpp @@ -321,11 +321,7 @@ std::vector parse_layers(FILE *fp, int z, unsigned x, unsigned y, int break; } - // json_read returns each parser token in sequence, including - // intermediate (still-incomplete) container nodes. Freeing those - // here would splice them out of the feature hash being built - // up, so only free `j` once we have processed a complete - // Feature (or `jp->root` when the stream ends). + // Only complete Features are freed; see plugin.cpp::parse_feature. json_object *type = json_hash_get(j, "type"); if (type == nullptr || type->type != JSON_STRING) { continue; diff --git a/unit.cpp b/unit.cpp index 4044bfba..7a04e20b 100644 --- a/unit.cpp +++ b/unit.cpp @@ -138,14 +138,10 @@ TEST_CASE("line_is_too_small") { REQUIRE(line_is_too_small(dv, 0, 10)); } -// Regression test for the surrogate-decoding bug that compared the leftover -// outer-loop byte `c` against `0xdfff` instead of the parsed code unit `ch`. -// For a string like "\uD83D\uE000" (a valid high surrogate followed by a -// non-surrogate BMP code point) the buggy version would mis-classify -// U+E000 as a low surrogate and combine the two units into the four-byte -// UTF-8 sequence F0 9F 90 80 (U+1F400). The fixed version flushes the -// stale high surrogate as standalone CESU-8 (ED A0 BD) and then encodes -// U+E000 normally as EE 80 80. +// A high surrogate followed by a non-surrogate used to be combined into one +// code point, because the range check tested the outer-loop byte instead of +// the parsed code unit. The stale surrogate should come out as standalone +// CESU-8, then U+E000 encoded normally. TEST_CASE("jsonpull surrogate-pair regression", "[jsonpull][surrogate]") { json_pull_ptr jp = json_begin_string("\"\\uD83D\\uE000\""); json_object_ptr o = json_read_tree(jp); @@ -163,22 +159,13 @@ TEST_CASE("jsonpull surrogate-pair regression", "[jsonpull][surrogate]") { REQUIRE(o->string() != buggy); } -// geojson-loop.cpp calls json_free(j) after jfa->add_feature has -// serialized the feature, intending to drop the JSON subtree from the -// in-progress parse tree so that already-serialized features don't sit -// in memory while subsequent features are parsed. That intent was -// never tested; this test pins it down. The pre-fix behavior of -// json_free was a bare unique_ptr/shared_ptr reset that only dropped -// the caller's local reference; the parent container kept the subtree -// alive, so memory grew until the top-level parse completed. +// What geojson-loop does: free each feature once serialized, so they do not +// accumulate while the rest of the document is parsed. // -// Note what this shape can and cannot catch. Because json_read hands back -// each container as it completes, the node freed here is always the most -// recently added element of its parent, which is the one case the old -// element-count-vs-byte-count memmove got right (it moved zero bytes). -// Widening this test to more elements does not change that. The -// "json_free prunes a non-final element" case below is what pins the -// array-splicing fix. +// This shape cannot catch the array-splicing bug, at any size: json_read hands +// back each container as it completes, so the node freed here is always the +// last-added element of its parent, which the old memmove got right by moving +// zero bytes. "json_free prunes a non-final element" below covers that. TEST_CASE("json_free prunes a subtree from its parent", "[jsonpull][memory]") { json_pull_ptr jp = json_begin_string("[[1, 2], [3, 4], [5, 6]]"); @@ -222,15 +209,9 @@ TEST_CASE("json_free prunes a subtree from its parent", "[jsonpull][memory]") { REQUIRE(outer->array()[1]->array()[1]->number() == 6); } -// The companion case to the pruning test above: in a line-delimited -// stream, each feature returned by json_read is a top-level value -// with no parent, but the parser still owns it via jp->root. -// json_free must drop that parser reference too, otherwise the -// just-serialized feature would sit in memory until the next feature -// started parsing. Under the unique_ptr ownership model, the only -// owner is jp->root, so verifying that jp->root is empty after the -// json_free call is also a guarantee that the subtree itself has -// been destroyed. +// A top-level value has no parent, so json_free has to drop the parser's +// reference instead. jp->root being empty afterwards is also proof the subtree +// was destroyed, since jp->root was its only owner. TEST_CASE("json_free releases a top-level value held by the parser", "[jsonpull][memory]") { json_pull_ptr jp = json_begin_string(R"({"a": 1, "b": [2, 3]})"); @@ -255,12 +236,9 @@ TEST_CASE("json_free releases a top-level value held by the parser", "[jsonpull] REQUIRE(jp->root == nullptr); } -// json_disconnect() is the documented way to splice a subtree out of the -// parser's tree and take ownership of it so that it can outlive the -// json_pull it came from. Nothing in tippecanoe calls it today -- the -// filter loaders get the same guarantee from json_read_tree, which clears -// back-pointers on the way out -- so cover it here rather than leave a -// documented ownership primitive untested. +// Nothing in tippecanoe calls json_disconnect today -- the filter loaders get +// the same guarantee from json_read_tree -- so cover it here rather than leave +// a documented ownership primitive untested. TEST_CASE("json_disconnect hands a subtree to the caller", "[jsonpull][ownership]") { json_object_ptr taken; json_object *outer = nullptr; @@ -317,11 +295,8 @@ TEST_CASE("json_disconnect hands a subtree to the caller", "[jsonpull][ownership REQUIRE(taken->array()[1]->number() == 4); } -// Preserving the parent links inside a detached tree is what lets -// json_free() keep working on its interior nodes: json_free() finds a -// node's owner through o->parent, so a detached tree whose parent links -// had been cleared would silently ignore the request and leave the node -// in place. +// json_free finds a node's owner through o->parent, so clearing the interior +// parent links on detach would make this a silent no-op. TEST_CASE("json_free prunes an interior node of a detached tree", "[jsonpull][ownership]") { json_pull_ptr jp = json_begin_string("[[1, 2], [3, 4], [5, 6]]"); json_object_ptr tree = json_read_tree(jp); @@ -350,10 +325,8 @@ TEST_CASE("json_free prunes an interior node of a detached tree", "[jsonpull][ow REQUIRE(tree->array()[1]->array()[0]->number() == 5); } -// The hash case is not a removal: json_free() of a hash value leaves the -// key in place with a JSON_NULL stand-in, so that detaching one half of a -// pair cannot disturb the key/value pairing of the entries around it. The -// entry is only erased once both halves are gone. +// Not a removal: the key stays with a JSON_NULL stand-in so the surrounding +// pairs keep their alignment, and the entry goes only when both halves do. TEST_CASE("json_free of a hash value leaves a null placeholder", "[jsonpull][ownership]") { json_pull_ptr jp = json_begin_string(R"({"keep": 1, "drop": [2, 3]})"); json_object_ptr tree = json_read_tree(jp); @@ -379,20 +352,11 @@ TEST_CASE("json_free of a hash value leaves a null placeholder", "[jsonpull][own REQUIRE(json_hash_get(tree, "keep")->number() == 1); } -// The array-splicing fix. The old code removed an element with -// -// memmove(arr + i, arr + i + 1, length - i - 1) -// -// passing an element count where memmove wants a byte count. Pruning -// element 0 of an eight-element array therefore copied seven bytes over an -// eight-byte pointer, which on a little-endian machine whose heap pointers -// share a zero high byte left arr[0] == arr[1] -- one node owned twice, so -// a double free at teardown -- and dropped the last element, since the -// length was decremented without anything having moved into place. -// -// Asserting the identity of every survivor is what makes this discriminate; -// checking only the resulting size would not. Verified to fail against the -// pre-fix code. +// The array-splicing fix. The old code passed an element count to memmove +// where a byte count was wanted, so pruning element 0 of eight left +// arr[0] == arr[1] -- one node owned twice -- and dropped the last element. +// Asserting each survivor's identity is what discriminates; checking only the +// size would not. TEST_CASE("json_free prunes a non-final element", "[jsonpull][ownership]") { json_pull_ptr jp = json_begin_string("[[1], [2], [3], [4], [5], [6], [7], [8]]"); json_object_ptr tree = json_read_tree(jp); @@ -417,9 +381,7 @@ TEST_CASE("json_free prunes a non-final element", "[jsonpull][ownership]") { } } -// json_free of a hash *key*, the mirror of the value case above. Detaching -// one half of a pair leaves a JSON_NULL stand-in so the surrounding pairs -// keep their alignment; the entry only disappears once both halves are gone. +// The mirror of the value case above. TEST_CASE("json_free of a hash key, then of both halves", "[jsonpull][ownership]") { json_pull_ptr jp = json_begin_string(R"({"a": 1, "b": 2, "c": 3})"); json_object_ptr tree = json_read_tree(jp); @@ -449,9 +411,8 @@ TEST_CASE("json_free of a hash key, then of both halves", "[jsonpull][ownership] REQUIRE(json_hash_get(tree, "c")->number() == 3); } -// What the filter loaders and the -L / -E arguments do: pull several -// top-level values off one parser in sequence. Each detached tree has to -// survive both the next json_read_tree call and the parser's destruction. +// What the filter loaders and -L / -E do. Each detached tree has to survive +// the next json_read_tree call and the parser's destruction. TEST_CASE("repeated json_read_tree on a line-delimited stream", "[jsonpull][ownership]") { json_pull_ptr jp = json_begin_string("{\"n\": 1}\n{\"n\": 2}\n{\"n\": 3}\n"); @@ -475,9 +436,8 @@ TEST_CASE("repeated json_read_tree on a line-delimited stream", "[jsonpull][owne } } -// json_stringify on a partially-parsed tree, which is what json_context() -// prints on the error paths in geojson-loop / read_json / plugin. A hash -// whose last key has no value yet renders that slot as "...". +// What json_context() prints on the error paths: a hash whose last key has no +// value yet renders that slot as "...". TEST_CASE("json_stringify of a partially-parsed tree", "[jsonpull][stringify]") { json_pull_ptr jp = json_begin_string("{\"a\": [1, 2], \"b\":"); @@ -492,9 +452,8 @@ TEST_CASE("json_stringify of a partially-parsed tree", "[jsonpull][stringify]") REQUIRE(s == "{\"a\":[1,2],\"b\":...}"); } -// An embedded NUL is a legal part of a JSON string once values are -// std::string, so stringify has to walk the whole value rather than stop at -// the first NUL the way a c_str() loop would. +// Values are std::string now, so an embedded NUL is legal and stringify has to +// walk past it rather than stop the way a c_str() loop would. TEST_CASE("json_stringify keeps text after an embedded NUL", "[jsonpull][stringify]") { json_pull_ptr jp = json_begin_string("\"a\\u0000b\""); json_object_ptr o = json_read_tree(jp); @@ -507,11 +466,8 @@ TEST_CASE("json_stringify keeps text after an embedded NUL", "[jsonpull][stringi REQUIRE(s == "\"a\\u0000b\""); } -// A \uXXXX escape can only name a code point up to U+FFFF, and U+FFFF -// itself used to fall through the `< 0xFFFF` test into the four-byte -// branch, which emitted the overlong sequence F0 8F BF BF. check_utf8() -// only validates continuation-byte structure, so that invalid UTF-8 was -// copied into tiles unnoticed. +// U+FFFF used to fall through the `< 0xFFFF` test into the four-byte branch +// and come out as the overlong F0 8F BF BF, which check_utf8 does not catch. TEST_CASE("jsonpull encodes U+FFFF as three bytes", "[jsonpull][utf8]") { json_pull_ptr jp = json_begin_string("\"a\\uFFFFb\""); json_object_ptr o = json_read_tree(jp); @@ -520,8 +476,12 @@ TEST_CASE("jsonpull encodes U+FFFF as three bytes", "[jsonpull][utf8]") { REQUIRE(o != nullptr); REQUIRE(o->type == JSON_STRING); - REQUIRE(o->string() == "a\xEF\xBF\xBF" "b"); - REQUIRE(o->string() != "a\xF0\x8F\xBF\xBF" "b"); + REQUIRE(o->string() == + "a\xEF\xBF\xBF" + "b"); + REQUIRE(o->string() != + "a\xF0\x8F\xBF\xBF" + "b"); // The boundary below it, and a genuine supplementary code point built // from a surrogate pair, both keep their existing encodings. @@ -544,8 +504,8 @@ TEST_CASE("Polygon cleaning drops a hole that no ring can parent", "[wagyu]") { // through the "Could not properly place hole to a parent." handler in // clean_or_clip_poly instead of returning. static const std::vector>> rings = { - {{0, 5}, {5, 4}, {5, 1}, {4, 4}, {4, 2}, {7, 1}, {0, 5}}, - {{0, 5}, {7, 1}, {4, 2}, {4, 4}, {5, 1}, {5, 4}, {0, 0}, {0, 5}}, + {{0, 5}, {5, 4}, {5, 1}, {4, 4}, {4, 2}, {7, 1}, {0, 5}}, + {{0, 5}, {7, 1}, {4, 2}, {4, 4}, {5, 1}, {5, 4}, {0, 0}, {0, 5}}, }; drawvec geom;