Github user alanconway commented on the pull request:
https://github.com/apache/qpid-proton/pull/35#issuecomment-109412800
On Fri, 2015-06-05 at 11:13 -0700, Andrew Stitcher wrote:
> Some overall comments (from a not yet thorough) read through:
> I really don't like using set/get in a C++ API. It's not idiomatic
> C++ API style; it's not necessary; and it doesn't read as well (IMO)
> So instead of
> Proton::transport::setXXXX('blah')/Proton::transport::getXXXX(), can
> we please use
> Proton::transport::XXXX('blah')/Proton::transport::XXXX().
> We use idiomatic properties in Python we should use them in C++ too.
100% agree, will fix. Those get/sets in Qpid have annoyed me for years.
> I'm not sure that we really need to use the Pimpl pattern for this
> binding. The real need for a Pimpl is to insulate this API from
> changes to code that it depends on - in this case once the C API is
> stable then its own API stability requirements should ensure that
> nothing gets broken in the C++ API too. I may not have thought this
> through hard enough though [is that a sentence with too many 'ough's
> or what ;-) ]
> This will save the indirection and extra resource use that Pimpl
> brings, which would force 2 allocations for each of the structures.
Agreed. Cliff also raised this question. He added some extra state to
those classes to make things more user-friendly, but I think we should
push such "helper" functionality to separate classes and keep a layer
that is simply a C++ wrapper of a C pointer.
> This strikes me as important because the C++ API can be as efficient
> as the C API if we do this work well, and it will be much easier to
> write and read code in good C++ than C. So I think that no one should
> need to do any application work in C even for applications that need
> the full efficiency of low level control of the library.
Yup, agreed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
