pxf: keep a comment that no entry follows (#176)

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
 

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<pxf::Comment>& cs) {
+  void Comments(const std::string& ind,
+                const std::vector<pxf::Comment>& 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<pxf::EntryPtr>& es) {
     for (const auto& e : es) {

include/protowire/pxf/ast.h

@@ -180,7 +180,11 @@ struct Block {
   // else.
   bool name_quoted = false;
   std::vector<EntryPtr> entries;
-  std::vector<Comment> leading_comments;
+  std::vector<Comment>
+      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<Comment> trailing_comments;
 };
 // TypeEntry is `@type = "<type-url>"` 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<EntryPtr> entries;
+  std::vector<EntryPtr>
+      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<Comment> trailing_comments;
 };
 
 // Directive is a top-of-document `@<name> *(<prefix-id>) [{ ... }]`
@@ -237,6 +245,10 @@ struct Directive {
 struct DatasetRow {
   Position pos;
   std::vector<std::optional<ValuePtr>> 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<Comment> leading_comments;
 };
 
 // DatasetDirective is a `@dataset <type> ( 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<EntryPtr> entries;
   std::vector<Comment> 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<Comment> trailing_comments;
 };
 
 Position EntryPos(const EntryPtr& e);

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<DatasetRow> ParseRow(int start, int end, int* end_offset = nullptr) const;
+  StatusOr<DatasetRow> ParseRow(int end, int* end_offset = nullptr) const;
   std::optional<DatasetRow> 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;

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<size_t>(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<DatasetRow> 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<DatasetRow> DatasetReader::ParseRow(int end, int* end_offset) const {
+  return ParseDatasetRow(std::string_view(pending_).substr(0, end),
                          static_cast<int>(columns_.size()),
                          opts_.max_nesting_depth > 0 ? opts_.max_nesting_depth : kMaxNestingDepth,
                          MaxBytesLiteral(),
-                         PosAt(static_cast<size_t>(start)),
+                         base_,
                          end_offset);
 }
 
@@ -376,22 +378,22 @@ StatusOr<DatasetRow> DatasetReader::ParseRow(int start, int end, int* end_offset
 // boundary. Called only between rows.
 std::optional<DatasetRow> DatasetReader::TryWholeRow() {
   int end_offset = 0;
-  auto row = ParseRow(scan_pos_, static_cast<int>(pending_.size()), &end_offset);
+  auto row = ParseRow(static_cast<int>(pending_.size()), &end_offset);
   if (!row.ok()) return std::nullopt;
   Consume(static_cast<size_t>(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<int>(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<int>(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

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;
 }
 

src/pxf/keyed.cc

@@ -35,10 +35,11 @@ void CanonEntries(std::vector<EntryPtr>& 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<EntryPtr>& 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<Comment> DropKeyAssignments(std::vector<EntryPtr>& entries,
+                                        std::string_view key_name,
+                                        std::string_view entry_name) {
   std::vector<Comment> pending;
   std::vector<EntryPtr> out;
   out.reserve(entries.size());
@@ -61,6 +62,13 @@ void DropKeyAssignments(std::vector<EntryPtr>& 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<Comment> left, std::vector<Comment>* 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,14 +89,15 @@ 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));
         }
       }
     }
     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,14 +138,16 @@ 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<ValuePtr, BlockVal>(lv->elements[i]);
-    DropKeyAssignments(bv->entries, key_name, keys[i]);
+    std::vector<Comment> left = DropKeyAssignments(bv->entries, key_name, keys[i]);
     CanonEntries(bv->entries, fd->message_type());
     auto eb = std::make_unique<Block>();
     eb->pos = bv->pos;
     eb->end = bv->end;
     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));
     }