From a27d51d80e90e41a9d8b4485deffa42af0beede5 Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Fri, 6 Nov 2026 12:21:04 -0800 Subject: [PATCH 1/2] pxf: write a message's unknown fields straight into the Go-order stream (#158) WriteMessage serialized the unknown fields into a temporary string with UnknownFieldSet::SerializeToString and dropped its result. That call fails only past 2 GiB, but when it fails it leaves the string empty, so the unknown fields would vanish without a word. WireFormat::SerializeUnknownFields writes the same bytes into the CodedOutputStream directly: no copy, and no result to drop. The stream is a StringOutputStream, which can't fail. GoParity.UnknownFieldsSerializeAfterTheKnownOnes pins the bytes: a Leaf with number = 3 and two unknown fields serializes to its input. --- src/pxf/go_wire.cc | 6 +++--- test/pxf_go_parity_test.cc | 11 +++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/pxf/go_wire.cc b/src/pxf/go_wire.cc index d09cbb9..8cd1e47 100644 --- a/src/pxf/go_wire.cc +++ b/src/pxf/go_wire.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include namespace protowire::pxf { @@ -199,9 +200,8 @@ void WriteMessage(const pb::Message& msg, pb::io::CodedOutputStream* out) { r->ListFields(msg, &fields); std::sort(fields.begin(), fields.end(), GoFieldLess); for (const FieldDescriptor* fd : fields) WriteField(msg, fd, out); - std::string unknown; - r->GetUnknownFields(msg).SerializeToString(&unknown); - out->WriteString(unknown); + // Straight into the stream: no copy, and no result to drop (#158). + pb::internal::WireFormat::SerializeUnknownFields(r->GetUnknownFields(msg), out); } } // namespace diff --git a/test/pxf_go_parity_test.cc b/test/pxf_go_parity_test.cc index c69db80..a7c8b8d 100644 --- a/test/pxf_go_parity_test.cc +++ b/test/pxf_go_parity_test.cc @@ -189,6 +189,17 @@ TEST_F(GoParity, OneofMembersSerializeLast) { EXPECT_EQ(copy->ShortDebugString(), msg->ShortDebugString()); } +// Unknown fields follow the known ones, byte for byte (#158: they used to go +// through an unchecked SerializeToString). +TEST_F(GoParity, UnknownFieldsSerializeAfterTheKnownOnes) { + auto msg = New("diff.v1.Leaf"); + // number = 3, then field 99 (varint 5) and field 100 (bytes "zz"), unknown to Leaf. + const std::string wire("\x30\x03\x98\x06\x05\xa2\x06\x02zz", 10); + ASSERT_TRUE(msg->ParseFromString(wire)); + ASSERT_EQ(msg->GetReflection()->GetUnknownFields(*msg).field_count(), 2); + EXPECT_EQ(pxf::SerializeGoOrder(*msg), wire); +} + TEST_F(GoParity, SecretHookTakesEveryForm) { std::vector> got; pxf::UnmarshalOptions opts; From 191559507d6884030fa5d6b114fdfb83d5a878cd Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Fri, 6 Nov 2026 12:21:04 -0800 Subject: [PATCH 2/2] build: no compiler warnings from this repo's own code (#158) With Abseil's flood gone (#157), a Clang build showed 31 warnings from our code and the GCC 13 job showed 39. None was a bug. Each is now fixed at its source: - -Wmissing-field-initializers: FieldSpec's optional strings get default member initializers, and schema.cc passes Violation::detail at its four call sites. GCC and Clang skip members that already have one, such as bracket_required and annotation_form. - -Wunused-result: the tests assert SerializeToString and SerializeToCodedStream instead of dropping them, and the two ByteSizeLong calls that only fill the size cache are cast to void. - -Wdangling-else (GCC): braces around two EXPECTs under a bare if. - -Wcomment (GCC): check_decode's usage comment no longer ends lines with a backslash, which continued the comment onto the next line. - ld's "ignoring duplicate libraries: libgtest.a": the test targets link GTest::gtest_main only, which already links GTest::gtest. A clean local build (Apple clang 21) now warns only inside googletest's own header. 726/726 tests pass. --- cmd/check_decode/main.cc | 8 ++++---- src/pxf/schema.cc | 12 ++++++++---- test/CMakeLists.txt | 10 +++++----- test/pxf_any_test.cc | 4 ++-- test/pxf_carrier_test.cc | 6 +++--- test/pxf_corpus_test.cc | 2 +- test/pxf_decode_test.cc | 4 ++-- test/pxf_full_roundtrip_test.cc | 4 ++-- test/pxf_rewriter_test.cc | 4 +++- test/sbe_header_bounds_test.cc | 4 +++- 10 files changed, 33 insertions(+), 25 deletions(-) diff --git a/cmd/check_decode/main.cc b/cmd/check_decode/main.cc index 17f3f9d..dc94486 100644 --- a/cmd/check_decode/main.cc +++ b/cmd/check_decode/main.cc @@ -10,10 +10,10 @@ // // Contract: // -// check_decode --format \ -// --schema \ -// --proto \ -// --input \ +// check_decode --format +// --schema +// --proto +// --input // [--limit NAME=VALUE ...] // // Exit 0 → input was accepted (decode succeeded) diff --git a/src/pxf/schema.cc b/src/pxf/schema.cc index 1e3c283..e1c0aec 100644 --- a/src/pxf/schema.cc +++ b/src/pxf/schema.cc @@ -41,7 +41,8 @@ void WalkEnumsForFile(std::string_view path, out->push_back(Violation{std::string(path), std::string(v->full_name()), std::string(v->name()), - ViolationKind::kEnumValue}); + ViolationKind::kEnumValue, + /*detail=*/{}}); } } } @@ -58,7 +59,8 @@ void WalkEnumsForMessage(std::string_view path, out->push_back(Violation{std::string(path), std::string(v->full_name()), std::string(v->name()), - ViolationKind::kEnumValue}); + ViolationKind::kEnumValue, + /*detail=*/{}}); } } } @@ -241,7 +243,8 @@ void WalkMessages(std::string_view path, const pb::Descriptor* md, std::vectorpush_back(Violation{std::string(path), std::string(f->full_name()), std::string(f->name()), - ViolationKind::kField}); + ViolationKind::kField, + /*detail=*/{}}); } // A field declared with no options shares the default instance and can // carry no annotation. Most fields in a closure are like this, and this @@ -263,7 +266,8 @@ void WalkMessages(std::string_view path, const pb::Descriptor* md, std::vectorpush_back(Violation{std::string(path), std::string(o->full_name()), std::string(o->name()), - ViolationKind::kOneof}); + ViolationKind::kOneof, + /*detail=*/{}}); } } for (int i = 0; i < md->nested_type_count(); ++i) { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3c5cd81..1b16040 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -13,13 +13,13 @@ add_executable(protowire_detail_test detail_rfc3339_test.cc detail_duration_test.cc) target_link_libraries(protowire_detail_test PRIVATE - protowire_detail GTest::gtest GTest::gtest_main) + protowire_detail GTest::gtest_main) gtest_discover_tests(protowire_detail_test) if(TARGET protowire_pb AND EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/pb_test.cc) add_executable(protowire_pb_test pb_test.cc pb_stream_test.cc pb_utf8_test.cc pb_values_table_test.cc) target_link_libraries(protowire_pb_test PRIVATE - protowire_pb GTest::gtest GTest::gtest_main) + protowire_pb GTest::gtest_main) target_compile_definitions(protowire_pb_test PRIVATE TESTDATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/../testdata") gtest_discover_tests(protowire_pb_test) @@ -28,7 +28,7 @@ endif() if(TARGET protowire_envelope AND EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/envelope_test.cc) add_executable(protowire_envelope_test envelope_test.cc) target_link_libraries(protowire_envelope_test PRIVATE - protowire_envelope GTest::gtest GTest::gtest_main) + protowire_envelope GTest::gtest_main) gtest_discover_tests(protowire_envelope_test) endif() @@ -52,7 +52,7 @@ if(TARGET protowire_pxf) add_executable(protowire_pxf_test ${_pxf_test_srcs}) target_link_libraries(protowire_pxf_test PRIVATE protowire_pxf protobuf::libprotoc - GTest::gtest GTest::gtest_main) + GTest::gtest_main) target_compile_definitions(protowire_pxf_test PRIVATE TESTDATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/../testdata" PROTO_DIR="${CMAKE_CURRENT_SOURCE_DIR}/../proto" @@ -74,7 +74,7 @@ if(TARGET protowire_sbe) add_executable(protowire_sbe_test ${_sbe_test_srcs}) target_link_libraries(protowire_sbe_test PRIVATE protowire_sbe protobuf::libprotoc - GTest::gtest GTest::gtest_main) + GTest::gtest_main) target_compile_definitions(protowire_sbe_test PRIVATE PROTO_DIR="${CMAKE_CURRENT_SOURCE_DIR}/../proto" WKT_PROTO_DIR="${Protobuf_INCLUDE_DIR}") diff --git a/test/pxf_any_test.cc b/test/pxf_any_test.cc index cdbf290..6e0332b 100644 --- a/test/pxf_any_test.cc +++ b/test/pxf_any_test.cc @@ -217,8 +217,8 @@ payload { ASSERT_TRUE(protowire::pxf::Unmarshal(*encoded, msg2.get(), uopts).ok()); std::string s1, s2; - msg1->SerializeToString(&s1); - msg2->SerializeToString(&s2); + ASSERT_TRUE(msg1->SerializeToString(&s1)); + ASSERT_TRUE(msg2->SerializeToString(&s2)); EXPECT_EQ(s1, s2); } diff --git a/test/pxf_carrier_test.cc b/test/pxf_carrier_test.cc index f68173a..5d96b81 100644 --- a/test/pxf_carrier_test.cc +++ b/test/pxf_carrier_test.cc @@ -63,10 +63,10 @@ std::string Other() { struct FieldSpec { std::string name; pb::FieldDescriptorProto::Type type; - std::string carrier; - std::string bracket_default; // (pxf.default) at extension 1315 + std::string carrier = {}; + std::string bracket_default = {}; // (pxf.default) at extension 1315 bool bracket_required = false; - std::string type_name; + std::string type_name = {}; }; // Schema builds one message M in a pool of its own. diff --git a/test/pxf_corpus_test.cc b/test/pxf_corpus_test.cc index 1158ed6..8c61163 100644 --- a/test/pxf_corpus_test.cc +++ b/test/pxf_corpus_test.cc @@ -120,7 +120,7 @@ std::string Bytes(const pbuf::Message& m) { pbuf::io::StringOutputStream raw(&out); pbuf::io::CodedOutputStream coded(&raw); coded.SetSerializationDeterministic(true); - m.SerializeToCodedStream(&coded); + EXPECT_TRUE(m.SerializeToCodedStream(&coded)); return out; } diff --git a/test/pxf_decode_test.cc b/test/pxf_decode_test.cc index 807b996..86124e2 100644 --- a/test/pxf_decode_test.cc +++ b/test/pxf_decode_test.cc @@ -182,8 +182,8 @@ TEST_F(PxfDecode, RoundTrip) { auto msg2 = NewAllTypes(); ASSERT_TRUE(protowire::pxf::Unmarshal(*encoded, msg2.get()).ok()); std::string s1, s2; - msg->SerializeToString(&s1); - msg2->SerializeToString(&s2); + ASSERT_TRUE(msg->SerializeToString(&s1)); + ASSERT_TRUE(msg2->SerializeToString(&s2)); EXPECT_EQ(s1, s2); } diff --git a/test/pxf_full_roundtrip_test.cc b/test/pxf_full_roundtrip_test.cc index 05eec88..07529a2 100644 --- a/test/pxf_full_roundtrip_test.cc +++ b/test/pxf_full_roundtrip_test.cc @@ -105,7 +105,7 @@ Trip RunFullPipeline(FullRoundTrip* f, const std::string& src) { t.m1 = f->NewAllTypes(); EXPECT_TRUE(protowire::pxf::Unmarshal(src, t.m1.get()).ok()); // Force size cache (required before SerializeWithCachedSizes). - t.m1->ByteSizeLong(); + (void)t.m1->ByteSizeLong(); SerializeDeterministic(*t.m1, &t.bin1); t.m2 = f->NewAllTypes(); @@ -117,7 +117,7 @@ Trip RunFullPipeline(FullRoundTrip* f, const std::string& src) { t.m3 = f->NewAllTypes(); EXPECT_TRUE(protowire::pxf::Unmarshal(t.text1, t.m3.get()).ok()); - t.m3->ByteSizeLong(); + (void)t.m3->ByteSizeLong(); // the size cache, as for m1 SerializeDeterministic(*t.m3, &t.bin3); return t; diff --git a/test/pxf_rewriter_test.cc b/test/pxf_rewriter_test.cc index aa6420c..c609ac7 100644 --- a/test/pxf_rewriter_test.cc +++ b/test/pxf_rewriter_test.cc @@ -1380,7 +1380,9 @@ TEST(RewriterKeyedComments, AnonCommentsSurviveEveryEdit) { ASSERT_TRUE(pxf::Parse(got).ok()) << got; ++checked; for (const std::string& k : keys) { - if (between) EXPECT_TRUE(Has(got, "// before " + k + "\n")) << got; + if (between) { + EXPECT_TRUE(Has(got, "// before " + k + "\n")) << got; + } if (trail.empty() || k == gone) continue; bool ok = false; std::istringstream lines(got); diff --git a/test/sbe_header_bounds_test.cc b/test/sbe_header_bounds_test.cc index d61c468..69b700b 100644 --- a/test/sbe_header_bounds_test.cc +++ b/test/sbe_header_bounds_test.cc @@ -194,7 +194,9 @@ TEST(SbeHeaderBounds, NewCodec) { ADD_FAILURE() << "short header"; continue; } - if (tc.block_size != 0) EXPECT_EQ(Le16(*data, 0), tc.block_size) << "blockLength"; + if (tc.block_size != 0) { + EXPECT_EQ(Le16(*data, 0), tc.block_size) << "blockLength"; + } EXPECT_EQ(std::to_string(Le16(*data, 4)), schema_id) << "schemaId"; EXPECT_EQ(std::to_string(Le16(*data, 6)), version) << "version";