Skip to content

test: add map serialization round-trip coverage#297

Merged
ryanofsky merged 2 commits into
bitcoin-core:masterfrom
ViniciusCestarii:test-type-map
Jul 8, 2026
Merged

test: add map serialization round-trip coverage#297
ryanofsky merged 2 commits into
bitcoin-core:masterfrom
ViniciusCestarii:test-type-map

Conversation

@ViniciusCestarii

@ViniciusCestarii ViniciusCestarii commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Add test to ensure map serialization is working and will keep working. Useful for #285.

Also renames v_bool to vector_bool.

Best reviewed commit by commit.

@DrahtBot

DrahtBot commented Jun 11, 2026

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers
ACK ryanofsky
Concept ACK w0xlt
Stale ACK xyzconstant

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

@ViniciusCestarii

Copy link
Copy Markdown
Contributor Author

Forced-push 645d90b to include map on test.cpp for IWYU

@w0xlt

w0xlt commented Jun 12, 2026

Copy link
Copy Markdown

Concept ACK

@xyzconstant

Copy link
Copy Markdown
Contributor

Looks clean to me, tACK 645d90b

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 645d90b. Looks good but would slightly prefer to keep the generic Pair(T1, T2) type as described below

Comment thread test/mp/test/foo.capnp
message @0 :Text;
}

struct Pair(T1, T2) {

@ryanofsky ryanofsky Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "test: drop mapSize, now covered by map round-trip test" (490585f)

I'm not really a fan the commit dropping the mapsize test. It seem to me the generic Pair() type is good to keep because it could be used in other tests testing non-primitive maps, and also it makes helps make sure the code generator and library code works with generics in capnproto, not just non-generic types.

I also don't love renumbering the passDouble method. It seems a little cleaner to me to either fully follow the compatibility rules for updating schema files, or fully not follow them and just update all the field numbers to match field order. It doesn't seem like there is a rationale for numbering fields out of order and also not following compatibility rules.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, dropped the commit "test: drop mapSize, now covered by map round-trip test" 490585f. OBS: I still kept the StringIntPair because only pointer types (structs, lists, blobs, and interfaces) can be used as generic parameters.

@ViniciusCestarii

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing! forced push 463d073 dropping commit "test: drop mapSize, now covered by map round-trip test" 490585f because mapSize/Pair() test is useful for generic capnp coverage.

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 463d073

Thanks for the update!

@ryanofsky ryanofsky merged commit 1bd7025 into bitcoin-core:master Jul 8, 2026
13 checks passed
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.

5 participants