Add assume_64_bit_php codegen option to the PHP generator#27013
Add assume_64_bit_php codegen option to the PHP generator#27013sergiosalvatore wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
LOCI found performance insights on your pull request. |
bshaffer
left a comment
There was a problem hiding this comment.
Cool feature! This is LGTM from my side, but I have a few questions for core protobuf before accepting:
- Can you confirm that this feature is idiomatic with other
protocfeature flags? - Would it be possible to just drop support for 32-bit PHP builds? I don't know how important this is to continue to support, but it would certainly make our lives easier and our codebases cleaner to drop it.
Generated PHP code currently type-hints 64-bit integer fields as
`int|string` to cover 32-bit PHP builds, where values that overflow a
native `int` are returned as strings. On 64-bit PHP — the mainstream
deployment — the runtime always returns native `int`, so the union is
dead weight that noisies up static analysis, IDE autocompletion, and
docs for everyone who has committed to 64-bit.
This change adds an opt-in CLI option, analogous to `aggregate_metadata`:
protoc --php_out=assume_64_bit_php:. foo.proto
When the flag is present, INT64/UINT64/SINT64/FIXED64/SFIXED64 fields
emit `int` instead of `int|string` across setter signatures, PHPDoc
`@param` / `@return` / `@type`, and `RepeatedField<...>` generics
(which naturally collapse from `RepeatedField<int>|RepeatedField<string>`
to `RepeatedField<int>`). Wrapper types (`Int64Value`, `UInt64Value`)
honor the flag via `Options` threaded into the wrapper doc-comment
helpers. Default behavior (flag absent) is unchanged.
Scope:
- Codegen only. Runtime libraries (GPBUtil.php, convert.c) already
return native `int` on 64-bit PHP regardless of this flag.
- No change to checked-in generated WKT .php files; they remain on
default behavior unless regenerated with the flag.
- No new .proto file-level option.
- Descriptor-mode generation (`is_descriptor=true`) is intentionally
unchanged.
Unit tests cover the flag-on and flag-absent paths, scalar and repeated
64-bit fields, and verify the `int|string` union is fully absent when
the flag is enabled.
fd419f5 to
d64617d
Compare
|
Thanks for the reply @bshaffer! You make an excellent point about the naming. In light of that, I've changed the flag to With regard to dropping support for 32-bit PHP builds, that would be completely fine with me. However, I'm unsure of what the general strategy for such changes are within protobuf in general. My $0.02 might be to introduce it as a flag so folks like me can make use of it now, and indicate that 32-bit support (and the flag) will be removed in a future release. Again, I yield to any suggestions you might have. |
Generated PHP code currently type-hints 64-bit integer fields as
int|stringto cover 32-bit PHP builds, where values that overflow anative
intare returned as strings. On 64-bit PHP — the mainstreamdeployment — the runtime always returns native
int, so the union isdead weight that noisies up static analysis, IDE autocompletion, and
docs for everyone who has committed to 64-bit.
This change adds an opt-in CLI option, analogous to
aggregate_metadata:When the flag is present, INT64/UINT64/SINT64/FIXED64/SFIXED64 fields
emit
intinstead ofint|stringacross setter signatures, PHPDoc@param/@return/@type, andRepeatedField<...>generics(which naturally collapse from
RepeatedField<int>|RepeatedField<string>to
RepeatedField<int>). Wrapper types (Int64Value,UInt64Value)honor the flag via
Optionsthreaded into the wrapper doc-commenthelpers. Default behavior (flag absent) is unchanged.
Scope:
return native
inton 64-bit PHP regardless of this flag.default behavior unless regenerated with the flag.
is_descriptor=true) is intentionallyunchanged.
Unit tests cover the flag-on and flag-absent paths, scalar and repeated
64-bit fields, and verify the
int|stringunion is fully absent whenthe flag is enabled.