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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to