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 accessors for nested messages in Javanano #187

Closed
wants to merge 3 commits into from
Closed

Added accessors for nested messages in Javanano #187

wants to merge 3 commits into from

Conversation

ennerf
Copy link
Contributor

@ennerf ennerf commented Feb 1, 2015

This adds accessors for nested messages so that references can be kept in order to reduce garbage. This addresses Issue #167.

Note that this changes the default behavior and breaks backwards compatibility. How do you normally handle this? Would this go into a separate branch first?

@ennerf
Copy link
Contributor Author

ennerf commented Feb 1, 2015

Do you think setMessage(null) should be allowed in case someone wants to explicitely remove the reference?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Feb 2, 2015

Does this PR change the generated API of message fields? Although proto3 is still alpha, this nano implementation (ported from AOSP) is already used by many people both inside and outside Google. We cannot change the API or it will break existing users.

@ennerf
Copy link
Contributor Author

ennerf commented Feb 2, 2015

Yes it does change the API for nested messages generated with "accessors". I thought this is roughly what you had in mind in #167.

Were you only referring to the clear() function? If so, how would you decide whether or not to serialize a message? I can change the implementation if you could give me some guidance on what you'd like to see.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Feb 3, 2015

Sorry, I misunderstood what is already implemented. I was thinking accessors are already generated for message fields with optional_field_style=accessors. If that's not the case, making such a change will be API incompatible and we'll need to add another flag to make a smooth transition for old users. As there are already quite some flags in the nano implementation, we are still finalizing the plan about what flags to be kept, what flags to be removed and how we migrate existing users. Could you hold on this PR and wait until we have a finalized plan for all the nano options? Hopefully we can have something in the next month.

@ennerf
Copy link
Contributor Author

ennerf commented Feb 3, 2015

Sure, just let me know whenever you decide on something. If you end up adding another flag for this, it'd probably be best to create accessors for all the other reference fields as well.

I think the current behavior of "optional_field_style=accessors" is a bit odd. It generates accessors for primitive and enum fields, but leaves repeated and message fields to their defaults. This results in inconsistent syntax and prohibits welcome memory optimizations.

@acozzette
Copy link
Member

I'm going to close this pull request because we no longer support the javanano implementation.

@acozzette acozzette closed this Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants