Repository navigation
build: no compiler warnings from this repo's own code (#158) - #163
Merged
Merged
Conversation
…am (#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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #158.
The build no longer warns about this repo's own code. Before, a Clang build showed 31 warnings and the GCC 13 CI job 39 warning lines, all of them in
src/,test/orcmd/. None was a bug. One could have become one: unknown fields inSerializeGoOrderwent through a serialize call whose failure was ignored.Changes
src/pxf/go_wire.cc(its own commit): unknown fields are written straight into the coded stream withWireFormat::SerializeUnknownFields. Before, they went through a temporary string with an uncheckedSerializeToString. That call fails only past 2 GiB, but a failure would have dropped the unknown fields silently. The output bytes are unchanged: the newGoParity.UnknownFieldsSerializeAfterTheKnownOnespins them.-Wmissing-field-initializers:FieldSpec's optional strings (test-only) get default member initializers.schema.ccpassesViolation::detailat its four call sites, so the publicViolationheader is unchanged. Both compilers already skip members that have a default, such asbracket_requiredandannotation_form.-Wunused-result: the tests assertSerializeToStringandSerializeToCodedStreaminstead of dropping them. The twoByteSizeLongcalls that only fill the size cache are cast tovoid.-Wdangling-else(GCC): braces around twoEXPECTs under a bareif.-Wcomment(GCC):check_decode's usage comment no longer ends its lines with\, which continued the comment onto the next line.Also fixed
ignoring duplicate libraries: '../lib/libgtest.a'(5 per build): the test targets link onlyGTest::gtest_main, which in googletest v1.15.2 already linksGTest::gtestpublicly.Evidence
Clean local build (Apple clang 21, Homebrew protobuf 36.2): the only compiler warning left is in googletest's own
gtest-printers.h(-Wcharacter-conversion), which is third-party. Thesearch path '/opt/homebrew/Cellar/llvm/18.1.8/lib' not foundlinker lines come from my shell'sLDFLAGS, not the repo. 726/726 tests pass.CI run 42287657025 (head 1915595), all jobs green. Warning lines per job log:
ubuntu-latest / gcc-13ubuntu-latest / clang-18macos-latest / appleclanggtest-printers.hwindows-latest / msvctext_format.hWhat's left is in third-party headers, outside the done-when.