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

added goproto_unrecognized support #33

Merged
merged 5 commits into from Dec 10, 2014

Conversation

Projects
None yet
2 participants
@anton-povarov
Copy link
Contributor

commented Dec 8, 2014

This implements the option to disable XXX_unrecognized fields generation.
According to our tests on huge number of objects - this eases GC work quite a bit, by removing extra pointers (and in conjunction with nullable=false - allows generated structures to be pointer-less - thus further simplifying GC process).
Should reduce Gc pauses and stuff.

Mostly discussed in private with Walter prior to github move :)

licence: do whatever you want with this code :)

@awalterschulze

This comment has been minimized.

Copy link

commented on test/example/example.proto in 25bf52f Dec 10, 2014

Could we remove this line, I think the example with R below is enough?

@awalterschulze

This comment has been minimized.

Could we add another test here with UnoM and OldUnoM messages which are the same as U and OldU except that their marshaler and unmarshaler generation is disabled.
This will test the reflection code's handling of Unrecognized.

I would like to see this if in proto/decode.go exercised

if !unrecField.IsValid() {
return nil
}

@awalterschulze

This comment has been minimized.

Copy link

commented on 25bf52f Dec 10, 2014

The rest of it looks perfect.
I also really like the extra new lines, especially in the String and GoString methods.

@anton-povarov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

something like this ?

@awalterschulze

This comment has been minimized.

Maybe just check that the Field2's are equal?

@awalterschulze

This comment has been minimized.

Copy link

commented on d88d2e7 Dec 10, 2014

This is exactly what I meant, great work :)

@awalterschulze

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

O and finally please do a make all before your next pull request, I think this should then be the last one.
In other words include all the generated code changes.

@anton-povarov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

also - found a bug in these tests, last unmarshal goes into wrong message, hang on.

@anton-povarov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

ok, this changes both tests for U and UnoM to actually check for message contents correctly.

  1. always initializes field that is supposed to be lost (in Populate* it's gated behind rand, so not reliable)
  2. unmarshals into older2 in the end, as it's supposed to
  3. checks that field1 is lost
  4. restores field1 in older2
  5. checks that messages are the same then

should be about right

@awalterschulze

This comment has been minimized.

Copy link

commented on 02b4c1a Dec 10, 2014

LGTM

@awalterschulze

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

Which version of protoc did you use to generate the protos with make all.
It seems all the comments in descriptor.pb.go and plugin.pb.go etc were deleted?

@anton-povarov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

yeah, that was 2.4.1, i'll regenerate with 2.6.1 ?

@awalterschulze

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

One final, final thing, sorry about this
gogoproto/doc.go
Please add one line of documentation right under goproto_extensions_map
Please also add the (beta) brackets, since we have not run this in the field for very long.

@anton-povarov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

yep.

awalterschulze added a commit that referenced this pull request Dec 10, 2014

Merge pull request #33 from anton-povarov/master
added goproto_unrecognized support

@awalterschulze awalterschulze merged commit 342974b into gogo:master Dec 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.