Hi Jisi and Jason, I'd like you to do a code review. This is the ...
13 years, 10 months ago
(2011-02-01 19:03:54 UTC)
#1
Hi Jisi and Jason,
I'd like you to do a code review. This is the implementation of the generated
visitors which I mentioned on the mailing list. Feel free to wait for some
discussion before actually digging into this review, if you'd prefer.
What's your plan for extension and unknown field support? The latter seems easier since unknown ...
13 years, 10 months ago
(2011-02-10 19:35:49 UTC)
#2
What's your plan for extension and unknown field support? The latter seems
easier since unknown fields have a nice public interface, but the extensions
support might be more important: since one of the applications is to work with
union types, and one of the solutions we've been pushing people toward is
extensions, I wonder if people will have a need for the extension support. This
is all wild conjecture; maybe we can just continue that in the discussion thread
and hopefully some users will respond. I'm happy moving forward without
extensions and unknown fields.
http://codereview.appspot.com/4077052/diff/1/src/google/protobuf/compiler/cpp...
File src/google/protobuf/compiler/cpp/cpp_unittest.cc (right):
http://codereview.appspot.com/4077052/diff/1/src/google/protobuf/compiler/cpp...
src/google/protobuf/compiler/cpp/cpp_unittest.cc:1002: // Test walker -> filler.
Should visit sub-objects directly, not use guides.
I wonder if this behavior is something that should be tested. While the Reader
should call the Guide methods to keep with the streaming paradigm, it doesn't
really matter for the Walker. It's more an optimization so that people who don't
need to apply a visitor to the child can just use the full generated message.
I had considered adding a comment that the code generator should check if the
child message has visitors when producing Walker code as well, and use the
Visit*Guide methods instead. I decided against it since visitors could have the
Visit* method do that if they wanted to stick with the visitor pattern down the
tree. After seeing this test I figured I'd bring it up: maybe Walker should just
use the visitors whenever available as well?
http://codereview.appspot.com/4077052/diff/1/src/google/protobuf/wire_format_...
File src/google/protobuf/wire_format_lite_inl.h (right):
http://codereview.appspot.com/4077052/diff/1/src/google/protobuf/wire_format_...
src/google/protobuf/wire_format_lite_inl.h:577: MessageType message;
Why does this method copy the message from the guide rather than using a
WriterType as above for the group case?
Issue 4077052: Implement generated visitor classes in C++.
Created 13 years, 10 months ago by kenton
Modified 11 years, 9 months ago
Reviewers: Jisi Liu, jasonh, chen3feng
Base URL: http://protobuf.googlecode.com/svn/trunk/
Comments: 3