Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc-gen-go: simplify generated oneof implementation #708

Closed
dsnet opened this issue Sep 15, 2018 · 1 comment
Closed

protoc-gen-go: simplify generated oneof implementation #708

dsnet opened this issue Sep 15, 2018 · 1 comment

Comments

@dsnet
Copy link
Member

dsnet commented Sep 15, 2018

The logic to generate support for oneofs are a significant source of complexity:

  • It is complexity to maintain in the generator.
  • It is complexity in the generated code.
  • It is complexity in the proto package as the generated code depends on the following types, functions, variables, and constants: Buffer, SizeVarint, ErrInternalBadWireType, WireBytes, WireEndGroup, WireFixed, WireStartGroup, and WireVarint.
  • It exposes a pseudo-internal method XXX_OneofFuncs.

However, this does not need to be the case:

  • The proto runtime, since switching to the table-driven approach, uses XXX_OneofFuncs in marshal and unmarshal only to obtain type information about the wrappers.

Since the type information is the only information needed, we can accomplish this in a cleaner way. Instead of adding the method, we add an unexported, zero-length field with all the wrapper types:

type FooMessage struct {
    // other fields of FooMessage as usual
    xxx_oneofWrappers [0]struct{
        Oneof_F_Bool
        Oneof_F_Int32
        Oneof_F_Int64
        Oneof_F_Fixed32
        Oneof_F_Fixed64
        Oneof_F_Uint32
        Oneof_F_Uint64
        Oneof_F_Float
        Oneof_F_Double
        Oneof_F_String
        Oneof_F_Bytes
        Oneof_F_Sint32
        Oneof_F_Sint64
        Oneof_F_Enum
        Oneof_F_Message
        Oneof_FGroup
        Oneof_F_Largest_Tag
        Oneof_Value
    }
}

This has the following properties:

  • It removes the need for the exported XXX method (see protoc-gen-go: unexport XXX_ fields from generated types #276). In fact, this approach keeps all of the type information hidden. Using Go reflection, you can obtain a list of all the oneof wrappers types from the type information in the message alone (no need to dynamically call methods).
  • It simplifies the implementation of proto since we don't have to type assert for a method with a relatively complex signature.
  • unsafe.Sizeof(FooMessage{}) remains unchanged
  • The generated code is significantly simpler.

\cc @neild

@dsnet dsnet added the api-v2 label Sep 15, 2018
dsnet added a commit that referenced this issue Nov 26, 2018
The marshaler, unmarshaler, and sizer functions are unused ever since
the underlying implementation was switched to be table-driven.
Change the function to only return the wrapper structs.

This change:
* enables generated protos to drop dependencies on certain proto types
* reduces the size of generated protos
* simplifies the implementation of oneofs in protoc-gen-go

Updates #708
dsnet added a commit that referenced this issue Nov 27, 2018
…760)

The marshaler, unmarshaler, and sizer functions are unused ever since
the underlying implementation was switched to be table-driven.
Change the function to only return the wrapper structs.

This change:
* enables generated protos to drop dependencies on certain proto types
* reduces the size of generated protos
* simplifies the implementation of oneofs in protoc-gen-go

Updates #708
dsnet added a commit that referenced this issue Nov 27, 2018
The marshaler, unmarshaler, and sizer functions are unused ever since
the underlying implementation was switched to be table-driven.
Change the function to only return the wrapper structs.

This change:
* enables generated protos to drop dependencies on certain proto types
* reduces the size of generated protos
* simplifies the implementation of oneofs in protoc-gen-go

Updates #708

Change-Id: I845c9009bc0236d1b51d34b014dc3e184303c0f2
Reviewed-on: https://go-review.googlesource.com/c/151357
Reviewed-by: Damien Neil <[email protected]>
dsnet added a commit that referenced this issue Nov 27, 2018
…760)

The marshaler, unmarshaler, and sizer functions are unused ever since
the underlying implementation was switched to be table-driven.
Change the function to only return the wrapper structs.

This change:
* enables generated protos to drop dependencies on certain proto types
* reduces the size of generated protos
* simplifies the implementation of oneofs in protoc-gen-go

Updates #708

Change-Id: I5d45dacc72549864a0fe5b2bf27de9a31ea91607
Cherry-Pick: github.com/golang/protobuf@8d0c54c1246661d9a51ca0ba455d22116d485eaa
Original-Author: Joe Tsai <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/151399
Reviewed-by: Damien Neil <[email protected]>
@dsnet
Copy link
Member Author

dsnet commented Jul 8, 2019

@dsnet dsnet closed this as completed Jul 8, 2019
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant