Code review comment for lp:~nknotts/dccl/encode-decode-iterators

Revision history for this message
toby schneider (tes) wrote :

This makes sense to me.

Minor style request: could you move the implementation of these methods to the bottom of codec.h after the declaration of dccl::Codec? (I want to keep the declaration of dccl::Codec clean):

1. template<typename CharIterator> unsigned id(CharIterator begin, CharIterator end)
2. template <typename CharIterator> void decode(CharIterator begin, CharIterator end, google::protobuf::Message* msg, bool header_only = false)

Otherwise should be fine. I'll give it one more detailed look over after you make that change and then I'll merge it in.

Also, dccl_test_ccl and dccl_test_arithmetic run fine on my machine for both lp:dccl/3.0 and this branch. I don't know if they are properly handling the .dylib extension on OSX (I think this is what you're using?). If you could open a separate bug report on those with the command line output when you run them, that'd be great.

Thanks-
-Toby

« Back to merge proposal