Skip to content

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

Merged
trendvidia merged 2 commits into
mainfrom
trailing-comments-176
Nov 7, 2026
Merged

trendvidia merged 2 commits into
mainfrom
trailing-comments-176

Conversation

@bfrancojr

@bfrancojr bfrancojr commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #176.

cpp now keeps a comment that no entry follows, as protowire-go does since protowire-go#283 (Go's #279). Without this, the differential against Go main fails on every PR, the release PR #175 included.

What changes for a reader of the AST

  • A comment at the end of a document goes to the new Document::trailing_comments. It was dropped.
  • A comment before a block's } goes to the new Block::trailing_comments or BlockVal::trailing_comments. It was dropped, or moved out of the block onto the next entry.
  • Comments before a dataset row's (, and inside the row, go to the new DatasetRow::leading_comments. They went to the next directive, or were dropped. Parse and DatasetReader set them the same way. The reader now parses a row from the start of its buffer, as Go's parseRow does, and the parser mode that dropped those comments is gone.
  • FormatDocument writes each where it was. A comment inside a row comes out before the row, since a row is written on one line. CanonicalizeKeyed keeps the comments through the blocks it builds, and the Rewriter's copies keep them too.
  • Comments inside a list still move after it, as in Go (protowire-go#282).

Evidence

  • scripts/go_differential.sh with spec dd8e221 and Go b3c210f: before this change, ParseTolerant differs (2 lines, both the # chords comment in testdata/differential/rewrite/settings.pxf); after it, no differences. The dumpers on both sides (cmd/pxf_diff, tools/godiff) now print the new fields, and Go's output is one line longer than before, which is that comment.
  • New test/pxf_closing_comments_test.cc (4 tests) and Keyed.CanonicalizeKeepsClosingComments port Go's format_comments_test.go with the same documents and expected output. All 5 fail with the parser, formatter, keyed and reader changes reverted, and pass with them.
  • ctest passes 747 of 747 locally. clang-format 18 has been applied. Local clang-tidy 23 reports nothing on the 5 changed source files.
  • Speed: streaming a 300,000-row table, best of 5, run twice. Plain rows take 84.5 ms against main's 81 ms (+4%). Rows that each carry a trailing comment take 91.5 ms against 84 ms (+9%), since those comments are now kept rather than skipped.
  • CI on f82e2fc (head): all 11 checks pass, including the differential against protowire-go (now main b3c210f), CodeQL, clang-tidy 18, ASan + UBSan and Windows.

Not ported

Go's new FuzzFormatDocumentFixedPoint fuzzer is not ported: this repo has no fuzz targets at all (no LLVMFuzzerTestOneInput anywhere). Every new test does check the format → parse → format fixed point.

Then

Once this merges, I'll rebase the release PR #175 so 2.1.0 includes it.

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.
@trendvidia
trendvidia merged commit e874165 into main Nov 7, 2026
11 checks passed
@trendvidia
trendvidia deleted the trailing-comments-176 branch November 7, 2026 11:44
@bfrancojr bfrancojr mentioned this pull request Nov 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pxf: keep a comment that no entry follows, as protowire-go#283 does (TrailingComments, DatasetRow.LeadingComments)

2 participants