size: fix compile error for a fixed-size convert shim (#446) - #447
Merged
klauspost merged 2 commits intoAug 31, 2026
Conversation
A "msgp:shim T as:BASE mode:convert" directive made the size generator emit a Msgsize that declared a temporary for the converted base value and never assigned it. For a fixed-size base (int64, float64, ...) the size is a compile-time constant that ignores the temporary, so the generated code failed to compile with "declared and not used". For a fixed-size base, emit the constant size directly and skip the temporary. Variable-size bases are intentionally left as-is: computing their exact size would require calling the shim in Msgsize, which can be more expensive than the resulting under-allocation. Add a fixed-size (int64) convert shim fixture and a round-trip test. Fixes tinylib#446
klauspost
approved these changes
Aug 14, 2026
klauspost
left a comment
klauspost
left a comment
Collaborator
There was a problem hiding this comment.
No show-stoppers, but a few easy wins.
Take both suggestions from the review: early-return in gBase so the non-fixed path is not indented, drop the bug narrative from the comments, and cover pointer, map and slice fields in the convert roundtrip. The added map and slice fields make ConvertInt non-comparable, so the test compares with reflect.DeepEqual.
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
2 participants
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 #446.
Problem
A
//msgp:shim T as:BASE using:to/from mode:convertdirective makes the size generator emit aMsgsize()that declares a temporary for the converted base value and never assigns it. For a fixed-size base (int64,float64, ...)basesizeExprreturns a compile-time constant that ignores the temporary, so the generated code fails to compile:Fix
In
(*sizeGen).gBase, split themode:convertbranch onfixedSize(b.Value). For a fixed-size base the wire size is a constant, so emit it directly (s.addConstant(basesizeExpr(b.Value, "", b.BaseName()))) and skip the temporary entirely.Variable-size bases are intentionally left as-is (they compile and only under-report the size). Per the discussion on #446, computing their exact size in
Msgsizewould require calling the shim, which can be more expensive than the resulting under-allocation - so this PR fixes the compile error without adding shim calls toMsgsize.Test
Added a fixed-size (
int64) convert-shim fixture (ConvertIntVal/ConvertInt) to_generated/convert.goand aTestConvertIntround-trip test to_generated/convert_test.go. Red-green verified:go generate ./_generated && go test ./_generatedfails to build (declared and not used) with the old generator and passes with the fixed one;ConvertInt.Msgsize()is now the constant1 + 4 + msgp.Int64Size.gofmtandgo vet ./genare clean.