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

C# Proto2 feature : Field presence and default values #4642

Conversation

ObsidianMinor
Copy link
Contributor

@ObsidianMinor ObsidianMinor commented May 17, 2018

This adds field presence and default values to the C# protoc compiler and library. This also includes some fixes for existing hacks in reflection that were created due to only supporting proto3 at the time of creation.

This PR also includes breaking changes. For required fields to work properly, a method is added the IMessage interface and breaks backwards compatibility with previous versions.

This is the first PR in a set of four PRs implementing all the changes required to support proto2 in the C# library and compiler.


This change is Reviewable

@xfxyjwf
Copy link
Contributor

xfxyjwf commented May 17, 2018

Assigned to @anandolee to view the code. I also linked it to https://reviewable.io/reviews/google/protobuf/4642#- because the code review function of github is horrible.

@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/field-presence branch 3 times, most recently from a791643 to 4adf0a9 Compare May 18, 2018 06:11
@xfxyjwf
Copy link
Contributor

xfxyjwf commented May 21, 2018

Ping @anandolee

@ObsidianMinor
Copy link
Contributor Author

Any updates @anandolee?

@xfxyjwf xfxyjwf added the c# label Jun 8, 2018
Copy link
Contributor

@anandolee anandolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR and sorry for late review

Remove the filter for proto2 in csharp_generator.cc :
https://github.com/google/protobuf/blob/master/src/google/protobuf/compiler/csharp/csharp_generator.cc#L69

The tests only has descriptor.proto which is proto2, should add more proto2 files for test

src/google/protobuf/compiler/csharp/csharp_field_base.cc Outdated Show resolved Hide resolved
@ObsidianMinor
Copy link
Contributor Author

I was going to add tests after I make the PRs for the other features. Since everything has already been implemented, after this gets pulled in I'll open the PR for extension support. And once that gets pulled in I'll open the PR for group support and all the tests for the other features since then they'd be all there. This would save work since to add support for this PR I'd have to create a custom tests proto and slowly add onto it with each PR rather than use the existing tests protos once.

@ObsidianMinor
Copy link
Contributor Author

Alright @anandolee, the proto2 check has been reintroduced and required handling has been changed to only be used for parsers.

Field presence currently uses ints for bit fields, however if you want I can make it use bytes instead of ints.

@anandolee
Copy link
Contributor

anandolee commented Jun 25, 2018

C++ is using one hasbit array for a message not has bytes for each field. Is it possible to do like that?

@ObsidianMinor
Copy link
Contributor Author

ObsidianMinor commented Jun 25, 2018

It is, however it has some cost. When you make a dynamically sized array in C#, it's considered another object on the heap which the garbage collector has to keep track of. In C++, that hasbit array is statically sized and is part of the memory of the original class. C# doesn't have features like these except in unsafe code, which we can't be sure a consumer will have as it's disabled by default. So to keep the code from allocating another array every time an object from our generated code is made, I made the ints part of the class itself.

@ObsidianMinor
Copy link
Contributor Author

@anandolee

@anandolee
Copy link
Contributor

anandolee commented Jun 29, 2018

Make sense to me. "I can make it use bytes instead of ints", I think bytes are better because they are named _hasBits in the code.

Another question, have you considered reflection usages for has method?

@ObsidianMinor
Copy link
Contributor Author

I have considered reflection usages, @anandolee, but I left it out since was unsure how to implement it since it'd have to be implemented it as well for proto3 and I don't know what a has method returns for proto3 fields.

I also take back what I said about using bytes instead of ints since bytes actually don't have bitwise operations, only ints and longs have them (as well as their unsigned counterparts). So each operation would convert the byte field to an int, then you'd have to cast back to an int for any assignment operations so that's two extra casts for each property set.

Copy link
Contributor

@anandolee anandolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For proto3, we can raise operation error for has method in reflection.

@ObsidianMinor
Copy link
Contributor Author

Comments and Has method reflection added @anandolee

@ObsidianMinor
Copy link
Contributor Author

Default values have been made private static readonly and have been removed from proto3. Clear methods have also been removed and field accessors have been adjusted accordingly @anandolee

Copy link
Contributor

@anandolee anandolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ObsidianMinor
Copy link
Contributor Author

I'll fix the distcheck. I'll also change how default string values are resolved since it seems codecvt isn't available.

@ObsidianMinor
Copy link
Contributor Author

@anandolee

@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/field-presence branch from 1d5992f to 72f2e08 Compare September 8, 2018 05:41
@anandolee
Copy link
Contributor

Friendly ping @ObsidianMinor , are you still working on this PR?

@ObsidianMinor
Copy link
Contributor Author

I am, @anandolee, I'm still wondering about your comments earlier about how

sub message may not required

since if this is true, then there would be no need to check any sub messages, including repeated fields and map fields.

Removed unused header method declarations
@ObsidianMinor
Copy link
Contributor Author

Doesn't look like I caused the failed Java builds.

@anandolee
Copy link
Contributor

anandolee commented Sep 24, 2018

Thanks for the change.
Just note: as proto2 is not fully supported, we still disable the proto2 codegen thus field presence is unable to test in this PR. API and implementation might be change in future PRs

Will merge the PR

@anandolee anandolee merged commit 54176b2 into protocolbuffers:master Sep 24, 2018
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing #5936, I gave this PR another pass and added a few comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants