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

Jump to bottom

internal::WireFormat::WireTypeForField fails to default proto3 repeated fields to packed #493

Closed
jskeet opened this issue Jun 12, 2015 · 7 comments
Closed

internal::WireFormat::WireTypeForField fails to default proto3 repeated fields to packed #493

jskeet opened this issue Jun 12, 2015 · 7 comments
Assignees
@TeBoring

Comments

@jskeet
Copy link
Contributor

jskeet commented Jun 12, 2015

WireFormat::MakeTag is implemented as:

return WireFormatLite::MakeTag(field->number(), WireTypeForField(field));

WireTypeForField in turn uses this check for "packed-ness":

if (field->options().packed())

That does not consider a repeated packable field in a proto3 file to be packed by default, whereas

if (field->is_packed())

does. I've made that simple change to WireTypeForField in my local clone, and can submit a PR if requested - but I don't know whether there's some intended reason for this behaviour.
@haberman
Copy link
Member

It's probably just an oversight. It was only recently that we changed the proto3 default to packed. I think the PR would be welcome!

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jun 12, 2015

Adding @TeBoring as he implemented the new packed behavior.

@jskeet
Copy link
Contributor Author

jskeet commented Jun 12, 2015

Okay. Will get to that on Monday then - will need to psych myself up for implementing my first unit test in C++ ;)

@jskeet
Copy link
Contributor Author

jskeet commented Jun 13, 2015

Okay, I've got a commit here:
jskeet@2b59317

It's not a pull request, however, as it breaks other tests. It's not clear to me whether that just
shows that those tests are broken, or whether there's something more subtle going on.
I'd suggest that someone more familiar with C++ than myself should take it from here :)

I can work around this in my C# codegen for the moment.

@TeBoring
Copy link
Contributor

The logic for paring proto3 packed repeated primitive fields is not right currently. Packed/Unpacked cannot be compatibly parsed. I will send a PR to fix it.

@TeBoring
Copy link
Contributor

Fixed in #500. @haberman There is a jruby test failed. Could you take a look?

@haberman haberman mentioned this issue Jun 16, 2015
Closed
@jskeet jskeet mentioned this issue Jun 19, 2015
Merged
@jtattermusch jtattermusch mentioned this issue Jun 19, 2015
Closed
jskeet added a commit to jskeet/protobuf that referenced this issue Jun 24, 2015
@jskeet
Revert the change to wire_format.h.
322ec53 
It seems too much code relies on the broken behaviour. See issue protocolbuffers#493.
Instead, we reimplement MakeTag just for C#, temporarily.
@TeBoring TeBoring closed this as completed Sep 8, 2015
taoso pushed a commit to taoso/protobuf that referenced this issue Aug 1, 2018
@dsnet
Revert "fix unnecessary rounding to float64 precision when JSON-marsh…
c65a041 
…aling durations (protocolbuffers#453)" (protocolbuffers#493)

The change does not handle negative values correctly.

This reverts commit 1e59b77.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@TeBoring TeBoring

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@haberman @jskeet @TeBoring @xfxyjwf and others