Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/pull/35#issuecomment-109383948
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.
* 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.
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.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
