GeneratedMessageReflection is an internal class used by the protobuf
implementation. Currently, users are not allowed to use it directly,
because we may change it at any time. You're suggesting that we promote it
to a public interface, which has implications with regard to maintenance
costs and implementation agility.
I'm open to considering making this change for performance purposes.
However, even them I'm hesitant to expose Repeated[Ptr]Field references
directly via this interface. I'd like to see what happens if we simply make
all of the existing public accessor methods "inline", so you could then do
something like:
int size = reflection->FieldSize(message, field);
for (int i = 0; i < size; i++) {
const Message& sub_message =
reflection->GeneratedMessageReflection::GetRepeatedMessage(message,
field, i);
// Do something with sub_message.
}
If GetRepeatedMessage is inline then I believe the above loop would be
nearly as efficient as iterating directly over a RepeatedPtrField. Note
that the funny syntax for the method call avoids making a virtual call.
BTW, the "has_templates" template you suggest would not work as you think --
how would you actually use it? What you would really need to use is
dynamic_cast. My golden rule of dynamic_cast is that it should only be used
for optimization, and you must provide an implementation for the case where
dynamic_cast always returns NULL. In your case, you are doing this, so that
should be fine.
On Sat, Feb 13, 2010 at 1:16 AM, Romain Francois <
[email protected]> wrote:
> Hello,
>
> Thanks for the answers.
>
> Maybe I should give some more background on why this is of interest to me.
> In RProtoBuf, we essentially only use the reflection api so that we can
> dynamically load new proto message types at runtime, etc ... we don't use
> protoc and therefore have no access to the generated classes.
>
> In the class "GeneratedMessageReflection", there are templates such as :
>
> template <typename Type>
> inline Type GetRepeatedField(const Message& message,
> const FieldDescriptor* field,
> int index) const;
>
> but they are private ? Why ?
>
> From what I can read of the code, methods like GetRepeatedInt32 get
> expanded out of :
>
> PASSTYPE GeneratedMessageReflection::GetRepeated##TYPENAME(
> \
> const Message& message, \
> const FieldDescriptor* field, int index) const { \
> USAGE_CHECK_ALL(GetRepeated##TYPENAME, REPEATED, CPPTYPE); \
> if (field->is_extension()) { \
> return GetExtensionSet(message).GetRepeated##TYPENAME( \
> field->number(), index); \
> } else { \
> return GetRepeatedField<TYPE>(message, field, index); \
> } \
> } \
>
> so doing things like this code
>
>
> for( int i=0; i<size; i++){
> INTEGER(res)[i] = (int) ref->GetRepeatedInt32( *message,
> fieldDesc,
> i ) ;
> }
>
> is going to be not as efficient as if I could directly iterate over the
> repeated field using RepeatedField::iterator
>
> Instead of extending the Reflection interface, what about making the
> templates public in GeneratedMessageReflection and then maybe use some sort
> of trait to indicate whether that the instance of Reflection I have access
> to has these templates.
>
>
> Something like :
>
>
> template <typename _T, _T _V> struct integral_constant {
> static const _T value = _V;
> typedef _T value_type;
> typedef integral_constant<_T,_V> type;
> };
> typedef integral_constant<bool, true> true_type;
> typedef integral_constant<bool, false> false_type;
>
> struct reflection_has_templates{} ;
> struct reflection_no_templates{} ;
>
> template <typename T> struct has_templates : public false_type{};
> template<> struct has_templates<GeneratedMessageReflection> : public
> true_type{} ;
>
> So that using this trait, I can dispatch between :
> - suboptimal implementation when the templates are not there
> - optimal implementation using RepeatedField::iterator when they are
> available.
>
> It adds nothing to the Reflection interface.
>
> Romain
>
>
>
> On 02/12/2010 10:37 PM, Kenton Varda wrote:
>
>> Yeah, it would add a lot of complication and reduce the flexibility of
>> the Reflection interface. We don't want to require repeated fields to
>> be implemented in terms of RepeatedField or RepeatedPtrField.
>>
>> On Fri, Feb 12, 2010 at 12:11 PM, Jason Hsueh <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>> +kenton
>>
>> Kenton may have a better answer, but I surmise that it's to avoid
>> tying the Reflection interface to implementation details. A Message
>> implementation might not use RepeatedField at all. The original
>> version of protobufs used a different class to represent repeated
>> fields, so it wouldn't have been possible to implement Reflection
>> for the original version if the interface required access to
>> RepeatedField. So maybe the reason is historical. Recent changes
>> have been made that would make this technically possible. However,
>> adding methods to get direct access to the RepeatedField would still
>> expand the Reflection interface quite a bit. I'll defer to Kenton on
>> that.
>>
>> On Fri, Feb 12, 2010 at 2:51 AM, Romain Francois
>> <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>>
>> Why not ? It seems reasonnable to want to use e.g. std::copy and
>> friends.
>>
>> On the documentation it says :
>>
>> "
>> Most users will not ever use a RepeatedField directly; they will
>> use the get-by-index, set-by-index, and add accessors that are
>> generated for all repeated fields
>> "
>>
>> What if I do want to use RepeatedField ?
>>
>> Romain
>>
>>
>> On 02/11/2010 06:50 PM, Jason Hsueh wrote:
>>
>> No, there isn't a way to get the RepeatedField from the
>> reflection
>> interface. You can only do so via the generated interface.
>>
>> On Thu, Feb 11, 2010 at 5:57 AM, Romain Francois
>> <[email protected]
>> <mailto:[email protected]>
>> <mailto:[email protected]
>> <mailto:[email protected]>>> wrote:
>>
>> Hello,
>>
>> How can I get hold of a RepeatedField object to manage a
>> repeated
>> field in C++.
>>
>> In RProtoBuf, we do a lot of :
>>
>> for( int i=0; i<size; i++){
>> INTEGER(res)[i] = (int) ref->GetRepeatedInt32( *message,
>> fieldDesc,
>> i ) ;
>> }
>>
>> where essentially the INTEGER macro gives a pointer to
>> the beginning
>> of the int array we are filling.
>>
>> I'd like to replace this using e.g std::copy
>>
>> RepeatedField field ;
>> std::copy( field.begin(), field.end(), INTEGER(res) ) ;
>>
>> but I can't find how to actually get hold of a
>> RepeatedField object.
>>
>> Is it possible ?
>>
>> Romain
>>
>
> --
> Romain Francois
> Professional R Enthusiast
> +33(0) 6 28 91 30 30
> http://romainfrancois.blog.free.fr
> |- http://tr.im/NrTG : Rcpp 0.7.5
> |- http://tr.im/MPYc : RProtoBuf: protocol buffers for R
> `- http://tr.im/KfKn : Rcpp 0.7.2
>
>
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/protobuf?hl=en.
