From 98148605b7be83bcb487d5f04ae7f46b6d97fcae Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 7 Nov 2026 03:10:17 -0800 Subject: [PATCH 1/2] pxf: keep a comment that no entry follows (#176) Ports protowire-go#283 (Go's #279). The parser attached a pending comment only to the entry after it, so a comment at the end of a document was dropped, one before a block's '}' was dropped or carried out of the block onto the next entry, and comments between dataset rows went to the next directive or were dropped. Document, Block and BlockVal gain trailing_comments, taken before the closing token is consumed. DatasetRow gains leading_comments, the comments before its '(' and inside it, taken by ParseTableRow itself. DatasetReader now parses a row from the start of its buffer, as Go's parseRow does, so the comments the scan skipped before the '(' are the row's, as Parse files them; the comment-dropping row parser is gone. FormatDocument writes the new comments in place, CanonicalizeKeyed carries them through the blocks it builds and keeps the comments of a key assignment it drops last in a block, and the Rewriter's clones copy them. The differential dumpers on both sides print the new fields. Against protowire-go b3c210f the differential failed on ParseTolerant before this and reports no differences after it. --- cmd/pxf_diff/main.cc | 10 +- include/protowire/pxf/ast.h | 19 ++- include/protowire/pxf/dataset_reader.h | 4 +- src/pxf/dataset_reader.cc | 36 ++--- src/pxf/format.cc | 5 + src/pxf/keyed.cc | 24 ++- src/pxf/parser.cc | 52 ++++--- src/pxf/rewriter.cc | 2 + test/CMakeLists.txt | 2 +- test/pxf_closing_comments_test.cc | 202 +++++++++++++++++++++++++ test/pxf_keyed_test.cc | 35 +++++ tools/godiff/main.go | 14 +- 12 files changed, 350 insertions(+), 55 deletions(-) create mode 100644 test/pxf_closing_comments_test.cc diff --git a/cmd/pxf_diff/main.cc b/cmd/pxf_diff/main.cc index 022886f..8a042f6 100644 --- a/cmd/pxf_diff/main.cc +++ b/cmd/pxf_diff/main.cc @@ -372,6 +372,7 @@ class AstDump { Comments("", x.leading_comments); for (const auto& r : x.rows) { out_ += "row " + Pos(r.pos) + "\n"; + Comments(" ", r.leading_comments); for (const auto& c : r.cells) { if (c.has_value()) { Value(" ", *c); @@ -382,6 +383,7 @@ class AstDump { } } Entries("", doc.entries); + Comments("", doc.trailing_comments, "trailing"); } private: @@ -394,9 +396,11 @@ class AstDump { for (size_t i = 0; i < v.size(); ++i) out += (i ? " " : "") + v[i]; return out; } - void Comments(const std::string& ind, const std::vector& cs) { + void Comments(const std::string& ind, + const std::vector& cs, + const char* label = "comment") { for (const auto& c : cs) { - out_ += ind + "comment " + Pos(c.pos) + " " + Quote(c.text) + "\n"; + out_ += ind + label + " " + Pos(c.pos) + " " + Quote(c.text) + "\n"; } } void Kind(const pxf::StringVal& v, const std::string&) { @@ -430,6 +434,7 @@ class AstDump { void Kind(const pxf::BlockVal& v, const std::string& ind) { out_ += "blockval\n"; Entries(ind + " ", v.entries); + Comments(ind + " ", v.trailing_comments, "trailing"); } void Value(const std::string& ind, const pxf::ValuePtr& v) { auto [start, end] = pxf::ValueSpan(v); @@ -456,6 +461,7 @@ class AstDump { out_ += "block " + Quote(e.name) + " " + Bool(e.name_quoted) + "\n"; Comments(ind, e.leading_comments); Entries(ind + " ", e.entries); + Comments(ind + " ", e.trailing_comments, "trailing"); } void Entries(const std::string& ind, const std::vector& es) { for (const auto& e : es) { diff --git a/include/protowire/pxf/ast.h b/include/protowire/pxf/ast.h index 7d10fba..a26468f 100644 --- a/include/protowire/pxf/ast.h +++ b/include/protowire/pxf/ast.h @@ -180,7 +180,11 @@ struct Block { // else. bool name_quoted = false; std::vector entries; - std::vector leading_comments; + std::vector + leading_comments; // trailing_comments are the comments after the last entry, before the + // closing '}' (all of an empty block's comments), as protowire-go has them + // (#176). + std::vector trailing_comments; }; // TypeEntry is `@type = ""` opening a block bound to // google.protobuf.Any (draft -01 "Any Blocks"). The rest of the block is a @@ -197,7 +201,11 @@ struct TypeEntry { struct BlockVal { Position pos; Position end; // just past the closing '}' - std::vector entries; + std::vector + entries; // trailing_comments are the comments after the last entry, before the + // closing '}' (all of an empty block's comments), as protowire-go has them + // (#176). + std::vector trailing_comments; }; // Directive is a top-of-document `@ *() [{ ... }]` @@ -237,6 +245,10 @@ struct Directive { struct DatasetRow { Position pos; std::vector> cells; + // leading_comments are the comments before this row (after the column + // list or the previous row) and inside it. FormatDocument writes a row + // on one line, so it writes them all before the row (#176). + std::vector leading_comments; }; // DatasetDirective is a `@dataset ( col1, col2, ... ) row*` entry @@ -308,6 +320,9 @@ struct Document { 0; // byte offset where the schema-typed body begins (after all leading directives) std::vector entries; std::vector leading_comments; + // trailing_comments are the comments after the document's last entry, + // directive or dataset row, which precede no entry of their own (#176). + std::vector trailing_comments; }; Position EntryPos(const EntryPtr& e); diff --git a/include/protowire/pxf/dataset_reader.h b/include/protowire/pxf/dataset_reader.h index fc0598a..70bc19d 100644 --- a/include/protowire/pxf/dataset_reader.h +++ b/include/protowire/pxf/dataset_reader.h @@ -140,9 +140,9 @@ class DatasetReader { Status Pull(size_t n); Status Fail(Status st); int MaxBytesLiteral() const; - StatusOr ParseRow(int start, int end, int* end_offset = nullptr) const; + StatusOr ParseRow(int end, int* end_offset = nullptr) const; std::optional TryWholeRow(); - Status RowError(int start) const; + Status RowError() const; RowScan ScanRow(int* start, int* end); RowScan ScanHeader(int* end); bool Complete(const Token& tok, int tok_end) const; diff --git a/src/pxf/dataset_reader.cc b/src/pxf/dataset_reader.cc index c946620..7bc0d92 100644 --- a/src/pxf/dataset_reader.cc +++ b/src/pxf/dataset_reader.cc @@ -240,7 +240,7 @@ Status DatasetReader::Next(DatasetRow* out) { int end = 0; switch (ScanRow(&start, &end)) { case RowScan::kRow: { - auto parsed = ParseRow(start, end); + auto parsed = ParseRow(end); Consume(static_cast(end)); if (!parsed.ok()) return Fail(parsed.status()); *out = std::move(parsed).consume(); @@ -251,7 +251,7 @@ Status DatasetReader::Next(DatasetRow* out) { // stopped the scan: the lexer may reject a token before its end (an // oversized bytes literal), and cut there it would read as // unterminated instead. - return Fail(RowError(start)); + return Fail(RowError()); case RowScan::kEnd: // The directive and what precedes it stay buffered, for Tail. finished_ = true; @@ -266,7 +266,7 @@ Status DatasetReader::Next(DatasetRow* out) { // At the end of the source only spaces and comments may remain; anything // else is a row cut short. if (src_eof_) { - if (start >= 0) return Fail(RowError(start)); + if (start >= 0) return Fail(RowError()); Consume(pending_.size()); finished_ = true; return Status::OK(); @@ -357,14 +357,16 @@ int DatasetReader::MaxBytesLiteral() const { : kMaxBytesLiteralLength; } -// ParseRow parses pending_ from `start` as one row, with the parser Parse -// uses, at its stream position. `end` bounds the bytes it may read. -StatusOr DatasetReader::ParseRow(int start, int end, int* end_offset) const { - return ParseDatasetRow(std::string_view(pending_).substr(start, end - start), +// ParseRow parses the row in pending_[:end] with the parser Parse uses, at +// its stream position. It starts at pending_[0], as protowire-go's parseRow +// does: only newlines and comments precede the row's `(` there, and those +// comments are the row's leading_comments, as Parse files them (#176). +StatusOr DatasetReader::ParseRow(int end, int* end_offset) const { + return ParseDatasetRow(std::string_view(pending_).substr(0, end), static_cast(columns_.size()), opts_.max_nesting_depth > 0 ? opts_.max_nesting_depth : kMaxNestingDepth, MaxBytesLiteral(), - PosAt(static_cast(start)), + base_, end_offset); } @@ -376,22 +378,22 @@ StatusOr DatasetReader::ParseRow(int start, int end, int* end_offset // boundary. Called only between rows. std::optional DatasetReader::TryWholeRow() { int end_offset = 0; - auto row = ParseRow(scan_pos_, static_cast(pending_.size()), &end_offset); + auto row = ParseRow(static_cast(pending_.size()), &end_offset); if (!row.ok()) return std::nullopt; Consume(static_cast(end_offset - base_.offset)); return std::move(row).consume(); } -// RowError is the error Parse reports for the row starting at pending_[start], -// which the scan found cannot close well. Every token before the one that -// stopped the scan is the same as Parse sees, so the row parser fails where -// Parse fails on the whole document. -Status DatasetReader::RowError(int start) const { - auto row = ParseRow(start, static_cast(pending_.size())); +// RowError is the error Parse reports for the row being scanned, which the +// scan found cannot close well. Only newlines and comments precede its `(` in +// the buffer, and every token before the one that stopped the scan is the +// same as Parse sees, so the row parser fails where Parse fails on the whole +// document. +Status DatasetReader::RowError() const { + auto row = ParseRow(static_cast(pending_.size())); if (!row.ok()) return row.status(); // Unreachable: the bytes hold no `)` that closes the row. - return Status::Error("pxf: DatasetReader: row at offset " + std::to_string(start) + - " neither closes nor fails"); + return Status::Error("pxf: DatasetReader: a row neither closes nor fails"); } // ScanRow finds the next row with the lexer Parse uses, resuming at diff --git a/src/pxf/format.cc b/src/pxf/format.cc index 60ef1d7..c03ff1e 100644 --- a/src/pxf/format.cc +++ b/src/pxf/format.cc @@ -145,6 +145,7 @@ class Formatter { } out_ += " {\n"; FormatEntries(b.entries, level + 1); + WriteComments(b.trailing_comments, level + 1); WriteIndent(level); out_ += "}\n"; } @@ -195,6 +196,7 @@ class Formatter { void FormatValueImpl(const BlockVal& v, int level) { out_ += "{\n"; FormatEntries(v.entries, level + 1); + WriteComments(v.trailing_comments, level + 1); WriteIndent(level); out_.push_back('}'); } @@ -295,6 +297,7 @@ void FormatDataset(const DatasetDirective& d, Formatter& f, std::string& out) { } out += ")\n"; for (const auto& row : d.rows) { + f.WriteComments(row.leading_comments, 0); out.push_back('('); for (size_t i = 0; i < row.cells.size(); ++i) { if (i > 0) out += ", "; @@ -356,6 +359,7 @@ std::string FormatDocument(const Document& doc) { } f.WriteComments(doc.leading_comments, 0); f.FormatEntries(doc.entries, 0); + f.WriteComments(doc.trailing_comments, 0); return out; } @@ -384,6 +388,7 @@ std::string FormatDocument(const Document& doc) { out.push_back('\n'); f.FormatEntries(doc.entries, 0); } + f.WriteComments(doc.trailing_comments, 0); return out; } diff --git a/src/pxf/keyed.cc b/src/pxf/keyed.cc index 38004b4..e9db154 100644 --- a/src/pxf/keyed.cc +++ b/src/pxf/keyed.cc @@ -35,10 +35,11 @@ void CanonEntries(std::vector& entries, const pb::Descriptor* desc); // redundant agreeing spelling of an entry's key — from entries. Disagreeing // or non-string assignments are kept (the document is invalid; formatting // must not silently change its meaning). Leading comments of a dropped -// assignment move to the next surviving entry. -void DropKeyAssignments(std::vector& entries, - std::string_view key_name, - std::string_view entry_name) { +// assignment move to the next surviving entry; when none follows, they are +// returned, for the block's trailing comments (#176). +std::vector DropKeyAssignments(std::vector& entries, + std::string_view key_name, + std::string_view entry_name) { std::vector pending; std::vector out; out.reserve(entries.size()); @@ -61,6 +62,13 @@ void DropKeyAssignments(std::vector& entries, out.push_back(std::move(e)); } entries = std::move(out); + return pending; +} + +// PrependComments puts `left` before the comments already in *to. +void PrependComments(std::vector left, std::vector* to) { + left.insert(left.end(), std::make_move_iterator(to->begin()), std::make_move_iterator(to->end())); + *to = std::move(left); } // CanonKeyedEntries normalizes the entries of a keyed block in place: @@ -81,6 +89,7 @@ void CanonKeyedEntries(Block* b, const pb::FieldDescriptor* fd, const pb::FieldD nb->name_quoted = a->key_quoted; nb->entries = std::move(bv->entries); nb->leading_comments = std::move(a->leading_comments); + nb->trailing_comments = std::move(bv->trailing_comments); eb = nb.get(); e = EntryPtr(std::move(nb)); } @@ -88,7 +97,7 @@ void CanonKeyedEntries(Block* b, const pb::FieldDescriptor* fd, const pb::FieldD } if (!eb) continue; // malformed entry; leave untouched if (eb->name_quoted && IsIdentifierSafeEntryName(eb->name)) eb->name_quoted = false; - DropKeyAssignments(eb->entries, key_name, eb->name); + PrependComments(DropKeyAssignments(eb->entries, key_name, eb->name), &eb->trailing_comments); CanonEntries(eb->entries, fd->message_type()); } } @@ -129,7 +138,7 @@ void CanonAnonymousKeyed(EntryPtr& entry, blk->leading_comments = std::move(n->leading_comments); for (size_t i = 0; i < lv->elements.size(); ++i) { auto* bv = GetMut(lv->elements[i]); - DropKeyAssignments(bv->entries, key_name, keys[i]); + std::vector left = DropKeyAssignments(bv->entries, key_name, keys[i]); CanonEntries(bv->entries, fd->message_type()); auto eb = std::make_unique(); eb->pos = bv->pos; @@ -137,6 +146,8 @@ void CanonAnonymousKeyed(EntryPtr& entry, eb->name = keys[i]; eb->name_quoted = !IsIdentifierSafeEntryName(keys[i]); eb->entries = std::move(bv->entries); + eb->trailing_comments = std::move(bv->trailing_comments); + PrependComments(std::move(left), &eb->trailing_comments); blk->entries.push_back(EntryPtr(std::move(eb))); } entry = EntryPtr(std::move(blk)); @@ -178,6 +189,7 @@ void CanonAssignment(EntryPtr& entry, Assignment* n, const pb::FieldDescriptor* blk->name = n->key; blk->entries = std::move(bv->entries); blk->leading_comments = std::move(n->leading_comments); + blk->trailing_comments = std::move(bv->trailing_comments); CanonKeyedEntries(blk.get(), fd, key_fd); entry = EntryPtr(std::move(blk)); } diff --git a/src/pxf/parser.cc b/src/pxf/parser.cc index 2b97efa..3b04c7e 100644 --- a/src/pxf/parser.cc +++ b/src/pxf/parser.cc @@ -49,19 +49,6 @@ class Parser { Advance(); } - // A parser that reads no comments before its first token. - struct DropLeadingCommentsTag {}; - Parser(DropLeadingCommentsTag, - std::string_view input, - int max_depth, - int max_bytes_literal, - Position start) - : lex_(input, start), max_depth_(max_depth), drop_comments_(true) { - lex_.SetMaxBytesLiteral(max_bytes_literal); - Advance(); - drop_comments_ = false; - } - struct TolerantTag {}; Parser(TolerantTag, std::string_view input, int max_depth, int max_bytes_literal) : lex_(input), max_depth_(max_depth), tolerant_(true) { @@ -134,7 +121,9 @@ class Parser { StatusOr ParseValue(int depth); StatusOr ParseList(int depth); StatusOr ParseBlockVal(int depth); - StatusOr> ParseBody(int depth, Position* end); + StatusOr> ParseBody(int depth, + Position* end, + std::vector* trailing); StatusOr ParseDirective(int* end_offset); StatusOr ParseProtoDirective(int* end_offset); StatusOr> ParseRowCell(); @@ -167,9 +156,6 @@ class Parser { int max_depth_ = kMaxNestingDepth; bool for_decoder_ = false; bool tolerant_ = false; - // Advance skips comments without keeping them, for a caller that reads - // none: DatasetReader's row parse, before the row's `(`. - bool drop_comments_ = false; std::vector errs_; Position err_pos_; // Just past the last significant token consumed: where a missing value @@ -183,7 +169,7 @@ void Parser::Advance() { current_ = lex_.Next(); if (current_.kind == TokenKind::kNewline) continue; if (current_.kind == TokenKind::kComment) { - if (!drop_comments_) comments_.push_back(Comment{current_.pos, std::string(current_.value)}); + comments_.push_back(Comment{current_.pos, std::string(current_.value)}); continue; } return; @@ -369,6 +355,8 @@ StatusOr Parser::ParseDocument() { auto entry = std::move(e).consume(); if (entry.has_value()) doc.entries.push_back(std::move(*entry)); } + // The comments after the last entry, directive or row (#176). + doc.trailing_comments = FlushComments(); return doc; } @@ -488,7 +476,7 @@ StatusOr> Parser::ParseEntry(int depth, bool allow_map_e return std::optional(EntryPtr(std::move(b))); } Advance(); // consume { - auto entries = ParseBody(depth + 1, &b->end); + auto entries = ParseBody(depth + 1, &b->end, &b->trailing_comments); if (!entries.ok()) return entries.status(); b->entries = std::move(entries).consume(); return std::optional(EntryPtr(std::move(b))); @@ -689,14 +677,19 @@ StatusOr Parser::ParseBlockVal(int depth) { return ValuePtr(std::move(bv)); } Advance(); // { - auto entries = ParseBody(depth + 1, &bv->end); + auto entries = ParseBody(depth + 1, &bv->end, &bv->trailing_comments); if (!entries.ok()) return entries.status(); bv->entries = std::move(entries).consume(); return ValuePtr(std::move(bv)); } -// ParseBody is entered just inside a `{` and leaves past its `}`. -StatusOr> Parser::ParseBody(int depth, Position* end) { +// ParseBody is entered just inside a `{` and leaves past its `}`. The +// comments after the last entry precede no entry, so no entry flushes them: +// they go to *trailing, taken before the `}` is consumed, since Advance would +// add the comments after it, which belong to what follows the block (#176). +StatusOr> Parser::ParseBody(int depth, + Position* end, + std::vector* trailing) { std::vector out; while (current_.kind != TokenKind::kRBrace && current_.kind != TokenKind::kEOF && !TolerantRowClose()) { @@ -714,6 +707,7 @@ StatusOr> Parser::ParseBody(int depth, Position* end) { auto entry = std::move(e).consume(); if (entry.has_value()) out.push_back(std::move(*entry)); } + *trailing = FlushComments(); if (current_.kind != TokenKind::kRBrace) { if (!tolerant_) { return Err(current_.pos, std::string("expected '}', got ") + TokenKindName(current_.kind)); @@ -926,6 +920,11 @@ StatusOr Parser::ParseTableDirective(int* end_offset) { // offset immediately past the closing `)` to *end_offset. StatusOr Parser::ParseTableRow(int expected, int* end_offset, bool read_past) { Position pos = current_.pos; + // The comments pending at the `(` and those inside the row are the row's + // leading_comments (#176). Left pending, they would pass to the next row or + // directive, or to the document's end. DatasetReader's row parser calls + // this too, so it files them as Parse does. + std::vector leading = FlushComments(); Advance(); // consume ( DatasetRow row; @@ -948,6 +947,13 @@ StatusOr Parser::ParseTableRow(int expected, int* end_offset, bool r std::string("expected ',' or ')' in @dataset row, got ") + DatasetTokenMsg()); } int eo = current_.pos.offset + 1; + // Taken before the `)` is consumed: Advance would add the comments after + // it, which belong to what follows the row. + std::vector inside = FlushComments(); + leading.insert(leading.end(), + std::make_move_iterator(inside.begin()), + std::make_move_iterator(inside.end())); + row.leading_comments = std::move(leading); // Consume the `)`, unless the caller reads nothing after the row: lexing on // would read the comments after it for nothing. if (read_past) Advance(); @@ -1187,7 +1193,7 @@ StatusOr ParseDatasetRow(std::string_view row, int max_bytes_literal, Position start, int* end_offset) { - Parser p(Parser::DropLeadingCommentsTag{}, row, max_depth, max_bytes_literal, start); + Parser p(row, max_depth, max_bytes_literal, start); if (p.current().kind != TokenKind::kLParen) { // Parse reports any token here, a lexical error included, as a // top-level entry beside the dataset. diff --git a/src/pxf/rewriter.cc b/src/pxf/rewriter.cc index 4728d7e..77673bc 100644 --- a/src/pxf/rewriter.cc +++ b/src/pxf/rewriter.cc @@ -88,6 +88,7 @@ EntryPtr CloneEntry(const EntryPtr& e) { n->name_quoted = b->name_quoted; n->entries = CloneEntries(b->entries); n->leading_comments = b->leading_comments; + n->trailing_comments = b->trailing_comments; return n; } return std::make_unique(*As(e)); @@ -115,6 +116,7 @@ ValuePtr CloneValue(const ValuePtr& v) { n->pos = b->pos; n->end = b->end; n->entries = CloneEntries(b->entries); + n->trailing_comments = b->trailing_comments; return n; } return std::visit( diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b609a89..8296dd6 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -38,7 +38,7 @@ if(TARGET protowire_pxf) pxf_null_test pxf_bignum_test pxf_features_test pxf_any_test pxf_fast_test pxf_full_roundtrip_test pxf_escapes_test pxf_annotations_test pxf_format_test pxf_tolerant_test pxf_check_test pxf_directive_test - pxf_schema_test pxf_schema_rules_test pxf_secret_test pxf_element_defaults_test pxf_result_directives_test pxf_dataset_reader_test pxf_dataset_scan_options_test + pxf_schema_test pxf_schema_rules_test pxf_secret_test pxf_element_defaults_test pxf_result_directives_test pxf_dataset_reader_test pxf_dataset_scan_options_test pxf_closing_comments_test pxf_proto_directive_test pxf_map_keys_test pxf_keyed_test pxf_hardening_test pxf_go_parity_test pxf_dataset_writer_test pxf_rewriter_test pxf_wkt_shape_test pxf_nan_test pxf_format_directives_test pxf_duration_block_test diff --git a/test/pxf_closing_comments_test.cc b/test/pxf_closing_comments_test.cc new file mode 100644 index 0000000..e048ac7 --- /dev/null +++ b/test/pxf_closing_comments_test.cc @@ -0,0 +1,202 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +// +// Comments that precede no entry — at the end of a document or of a block, +// and around dataset rows — survive Parse → FormatDocument in place (#176), +// as they do in protowire-go (#279, PR #283). Before, the parser attached a +// comment only to the entry after it, so a comment with none after it was +// dropped, or carried out of its block onto the next entry. The documents +// and expected outputs are protowire-go's (format_comments_test.go). + +#include "protowire/pxf/dataset_reader.h" +#include "protowire/pxf/format.h" +#include "protowire/pxf/parser.h" + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +namespace pxf = protowire::pxf; + +// FormatRoundTrip parses src and formats it, and requires the output to parse +// and to be a fixed point of formatting. +std::string FormatRoundTrip(const std::string& src) { + auto doc = pxf::Parse(src); + EXPECT_TRUE(doc.ok()) << doc.status().ToString() << "\n" << src; + if (!doc.ok()) return ""; + std::string out = pxf::FormatDocument(*doc); + auto again = pxf::Parse(out); + EXPECT_TRUE(again.ok()) << again.status().ToString() << "\n" << out; + if (!again.ok()) return out; + EXPECT_EQ(out, pxf::FormatDocument(*again)) << "formatting is not a fixed point"; + return out; +} + +std::vector Texts(const std::vector& cs) { + std::vector out; + out.reserve(cs.size()); + for (const auto& c : cs) out.push_back(c.text); + return out; +} + +TEST(ClosingComments, AtTheEnd) { + struct Case { + const char* name; + std::string src; + std::string want; // empty: the source itself + }; + const std::vector cases = { + // protowire-go#279's three documents. + {"after an entry", + "@type a.v1.M\nname = \"n\"\n# end of doc\n", + "@type a.v1.M\n\nname = \"n\"\n# end of doc\n"}, + {"after a directive", "@type a.v1.M\n@flag\n# end of doc\n", ""}, + {"after a row", "@dataset t.v1.T (id)\n(1)\n# end of doc\n", ""}, + {"a run of comments", + "@type a.v1.M\nname = \"n\" # inline\n# a\n// b\n/* c */\n", + "@type a.v1.M\n\nname = \"n\" # inline\n# a\n// b\n/* c */\n"}, + {"after @type alone", "@type a.v1.M\n# only\n", "@type a.v1.M\n\n# only\n"}, + }; + for (const auto& c : cases) { + EXPECT_EQ(FormatRoundTrip(c.src), c.want.empty() ? c.src : c.want) << c.name; + } + + auto doc = pxf::Parse("@type a.v1.M\nname = \"n\"\n# end of doc\n"); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + EXPECT_EQ(Texts(doc->trailing_comments), std::vector{"# end of doc"}); + const auto* a = std::get_if>(&doc->entries[0]); + ASSERT_NE(a, nullptr); + EXPECT_TRUE((*a)->leading_comments.empty()); +} + +TEST(ClosingComments, BeforeTheClosingBrace) { + const std::vector> cases = { + // Before, this comment moved out of m, onto y. + {"block then entry", "@type a.v1.M\n\nm {\n k: 1\n # end of m\n}\ny = 2\n"}, + {"last block", "@type a.v1.M\n\nb {\n x = 1\n # end of b\n}\n"}, + {"empty block", "@type a.v1.M\n\nb {\n # empty\n}\n"}, + {"block value", "@type a.v1.M\n\nb = {\n x = 1\n # end of b\n}\n"}, + {"nested", "@type a.v1.M\n\na {\n b {\n x = 1\n # end of b\n }\n # end of a\n}\n"}, + {"after the brace stays after", "@type a.v1.M\n\na {\n x = 1\n}\n# about y\ny = 2\n"}, + {"block cell", "@dataset t.v1.T (id, m)\n(1, {\n x = 1\n # end of m\n})\n"}, + }; + for (const auto& [name, src] : cases) EXPECT_EQ(FormatRoundTrip(src), src) << name; + + auto doc = pxf::Parse("@type a.v1.M\nm {\n k: 1\n # end of m\n}\ny = 2\n"); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto* m = std::get_if>(&doc->entries[0]); + ASSERT_NE(m, nullptr); + EXPECT_EQ(Texts((*m)->trailing_comments), std::vector{"# end of m"}); + const auto* y = std::get_if>(&doc->entries[1]); + ASSERT_NE(y, nullptr); + EXPECT_TRUE((*y)->leading_comments.empty()); +} + +TEST(ClosingComments, DatasetRows) { + const std::string src = "@dataset t.v1.T (id)\n# first\n(1)\n# between\n(2)\n# end\n"; + EXPECT_EQ(FormatRoundTrip(src), src); + + // A row is written on one line, so a comment inside it moves before it. + EXPECT_EQ(FormatRoundTrip("@dataset t.v1.T (a, b)\n(1, # one\n 2)\n(3, 4)\n"), + "@dataset t.v1.T (a, b)\n# one\n(1, 2)\n(3, 4)\n"); + + auto doc = pxf::Parse(src); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto& rows = doc->datasets[0].rows; + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(Texts(rows[0].leading_comments), std::vector{"# first"}); + EXPECT_EQ(Texts(rows[1].leading_comments), std::vector{"# between"}); + EXPECT_EQ(Texts(doc->trailing_comments), std::vector{"# end"}); +} + +// OneByteBuf serves its bytes one per read, so every token can be cut by a +// read's end. +class OneByteBuf : public std::streambuf { + public: + explicit OneByteBuf(std::string data) : data_(std::move(data)) {} + + protected: + int_type underflow() override { + if (gptr() != nullptr && gptr() < egptr()) return traits_type::to_int_type(*gptr()); + if (next_ >= data_.size()) return traits_type::eof(); + char* at = &data_[next_++]; + setg(at, at, at + 1); + return traits_type::to_int_type(*at); + } + + private: + std::string data_; + size_t next_ = 0; +}; + +struct RowComments { + pxf::Position pos; + std::vector comments; +}; + +std::vector StreamRows(std::istream* in) { + std::vector out; + auto tr = pxf::DatasetReader::Create(in); + EXPECT_TRUE(tr.ok()) << tr.status().ToString(); + if (!tr.ok()) return out; + for (;;) { + pxf::DatasetRow row; + auto st = (*tr)->Next(&row); + EXPECT_TRUE(st.ok()) << st.ToString(); + if (!st.ok() || (*tr)->Done()) break; + out.push_back({row.pos, row.leading_comments}); + } + return out; +} + +void ExpectSameComments(const std::vector& got, + const std::vector& want, + const std::string& how) { + ASSERT_EQ(got.size(), want.size()) << how; + for (size_t i = 0; i < want.size(); ++i) { + EXPECT_EQ(got[i].pos.offset, want[i].pos.offset) << how << " row " << i; + ASSERT_EQ(got[i].comments.size(), want[i].leading_comments.size()) << how << " row " << i; + for (size_t j = 0; j < want[i].leading_comments.size(); ++j) { + const auto& g = got[i].comments[j]; + const auto& w = want[i].leading_comments[j]; + EXPECT_EQ(g.text, w.text) << how << " row " << i; + EXPECT_EQ(g.pos.line, w.pos.line) << how << " row " << i; + EXPECT_EQ(g.pos.column, w.pos.column) << how << " row " << i; + EXPECT_EQ(g.pos.offset, w.pos.offset) << how << " row " << i; + } + } +} + +// DatasetReader files a row's comments as Parse does, read whole or a byte +// at a time. +TEST(ClosingComments, DatasetReaderRowComments) { + for (const char* body : {"# first\n(1, 2)\n# between\n/* b */\n(3, 4)\n# end\n", + "(1, # one\n 2)\n(3, 4) # after\n(5, 6)\n", + "(1, 2)\n# about U\n@dataset U (z)\n(9)\n"}) { + const std::string in = std::string("@dataset T (a, b)\n") + body; + auto doc = pxf::Parse(in); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto& want = doc->datasets[0].rows; + + std::istringstream whole(in); + ExpectSameComments(StreamRows(&whole), want, std::string("whole: ") + body); + OneByteBuf buf(in); + std::istream bytes(&buf); + ExpectSameComments(StreamRows(&bytes), want, std::string("one byte: ") + body); + } + + std::istringstream src("@dataset T (a, b)\n(1, 2)\n# between\n(3, 4)\n"); + auto rows = StreamRows(&src); + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(Texts(rows[1].comments), std::vector{"# between"}); +} + +} // namespace diff --git a/test/pxf_keyed_test.cc b/test/pxf_keyed_test.cc index 6800c69..a6a240f 100644 --- a/test/pxf_keyed_test.cc +++ b/test/pxf_keyed_test.cc @@ -517,4 +517,39 @@ TEST_F(Keyed, DiagnosticsToleratesNullDescriptor) { EXPECT_TRUE(protowire::pxf::KeyedDiagnostics(doc.document, nullptr).empty()); } +// CanonicalizeKeyed keeps a block's closing comments when it rewrites the +// block, and the comments of a redundant key assignment it drops last in a +// block (#176; protowire-go's TestCanonicalizeKeyed_KeepsClosingComments). +TEST_F(Keyed, CanonicalizeKeepsClosingComments) { + struct Case { + const char* name; + std::string src; + std::string want; + }; + const std::vector cases = { + {"dropped last key assignment", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # about the " + "key\n id = \"greeting\"\n }\n # end of children\n}\n", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # about the " + "key\n }\n # end of children\n}\n"}, + {"assignment form", + "@type keyed.v1.Node\n\nchildren = {\n greeting = {\n type = \"Label\"\n # end of " + "greeting\n }\n # end of children\n}\n", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # end of " + "greeting\n }\n # end of children\n}\n"}, + {"anonymous list", + "@type keyed.v1.Node\n\nchildren = [\n {\n type = \"L\"\n # about the key\n id = " + "\"g\"\n },\n {\n id = \"h\"\n # end of h\n }\n]\n", + "@type keyed.v1.Node\n\nchildren {\n g {\n type = \"L\"\n # about the key\n }\n h " + "{\n # end of h\n }\n}\n"}, + }; + for (const auto& c : cases) { + const pb::Descriptor* d = DescOf(c.src); + ASSERT_NE(d, nullptr) << c.name; + const std::string out = Fmt(c.src, d); + EXPECT_EQ(out, c.want) << c.name; + EXPECT_EQ(Fmt(out, d), out) << c.name << ": formatting is not a fixed point"; + } +} + } // namespace diff --git a/tools/godiff/main.go b/tools/godiff/main.go index 11f6fb2..18b0c9b 100644 --- a/tools/godiff/main.go +++ b/tools/godiff/main.go @@ -317,9 +317,15 @@ type astDump struct{ w *bufio.Writer } func pos(p pxf.Position) string { return fmt.Sprintf("%d:%d:%d", p.Line, p.Column, p.Offset) } -func (d astDump) comments(ind string, cs []pxf.Comment) { +func (d astDump) comments(ind string, cs []pxf.Comment) { d.labeled(ind, "comment", cs) } + +// trailing writes the comments that follow a block's or document's last +// entry (TrailingComments, protowire-go#279). +func (d astDump) trailing(ind string, cs []pxf.Comment) { d.labeled(ind, "trailing", cs) } + +func (d astDump) labeled(ind, label string, cs []pxf.Comment) { for _, c := range cs { - fmt.Fprintf(d.w, "%scomment %s %s\n", ind, pos(c.Pos), quote(c.Text)) + fmt.Fprintf(d.w, "%s%s %s %s\n", ind, label, pos(c.Pos), quote(c.Text)) } } @@ -355,6 +361,7 @@ func (d astDump) value(ind string, v pxf.Value) { case *pxf.BlockVal: fmt.Fprintf(d.w, "blockval\n") d.entries(ind+" ", v.Entries) + d.trailing(ind+" ", v.TrailingComments) } } @@ -378,6 +385,7 @@ func (d astDump) entries(ind string, es []pxf.Entry) { fmt.Fprintf(d.w, "block %s %v\n", quote(e.Name), e.NameQuoted) d.comments(ind, e.LeadingComments) d.entries(ind+" ", e.Entries) + d.trailing(ind+" ", e.TrailingComments) } } } @@ -399,6 +407,7 @@ func (d astDump) document(doc *pxf.Document) { d.comments("", x.LeadingComments) for _, r := range x.Rows { fmt.Fprintf(d.w, "row %s\n", pos(r.Pos)) + d.comments(" ", r.LeadingComments) for _, c := range r.Cells { if c == nil { fmt.Fprintf(d.w, " empty\n") @@ -409,6 +418,7 @@ func (d astDump) document(doc *pxf.Document) { } } d.entries("", doc.Entries) + d.trailing("", doc.TrailingComments) } // defaultTable is the same list in cmd/pxf_diff. The literals are a schema's, From f82e2fcc03c552bc9eb880e69da1d527200630e0 Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 7 Nov 2026 03:10:29 -0800 Subject: [PATCH 2/2] changelog: keep a comment that no entry follows (#176) --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index feb2a59..4f2aafd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ number says nothing about any other port. See the Releases section of - **`DatasetReader` returns a complete row from a stream that stays open** (#171), as protowire-go does. On a pipe, a socket or a file still being appended to, `Next` could wait for 4 KiB more input, or for the stream to end, before returning a row that had already arrived. The read took a full 4 KiB (`std::istream::read`), and the scan held a token until four more bytes followed it. `Next` now reads what the stream has, blocking only when nothing is buffered. It takes a token as complete as soon as the bytes the lexer looks at past it have arrived. Input that ends reads as before. - **A row's validator error and schema violation carry no row position** (#166), as protowire-go gives them. `DatasetReader::Scan` and `BindRow` put the row's position in front of every error, so a validator's refusal read `2:1: check: id: denied (...)` and a schema violation `2:1: PXF schema violations: ...`. Only a decode error is moved to the row now, and a hook's error from `on_secret_field` is one, so it keeps the row's position and its column. `Scan` also checks the column paths before the schema, as Go does, so a row type that breaks both gets the column's error. - **`DatasetReader::Scan` honours every option but `max_message_size`, and protowire-go is changing to match** (#166). Go's `Scan` ignored `skip_validate`, `skip_post_decode`, `discard_unknown`, `on_secret_field` and `validator`, while cpp's honoured them. The owner chose to change Go (trendvidia/protowire-go#280) and keep cpp's behaviour. Each option now has a test that binds the same row through `Scan` and `BindRow`, with the texts Go gives, and `dataset_reader.h` lists the options `Scan` uses. +- **A comment that no entry follows is kept** (#176), as protowire-go keeps it (protowire-go#279, PR #283). The parser attached a comment only to the entry after it. So a comment at the end of a document was dropped, and one before a block's closing `}` was dropped or carried out of the block onto the next entry. Comments between dataset rows went to the next directive, or were dropped at the end. `Document`, `Block` and `BlockVal` now have `trailing_comments`, and `DatasetRow` has `leading_comments`, which `Parse` and `DatasetReader` both set. `FormatDocument` writes each in place, `CanonicalizeKeyed` keeps them when it rewrites a keyed block, and `Rewriter` copies them. A comment inside a row now comes before the row. Comments inside a list still move after it (protowire-go#282). Streaming a 300,000-row table is about 4% slower, and 9% slower when each row carries a comment, since those comments are now kept. ## [2.0.0] — 2026-11-05