You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2014/05/27 18:30:28 UTC
Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/
-----------------------------------------------------------
Review request for qpid, Darryl Pierce and Rafael Schloming.
Bugs: PROTON-429
https://issues.apache.org/jira/browse/PROTON-429
Repository: qpid
Description
-------
Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
Points/Questions:
1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
Diffs
-----
/proton/trunk/proton-c/bindings/perl/perl.i 1596912
/proton/trunk/proton-c/bindings/php/php.i 1596912
/proton/trunk/proton-c/bindings/python/python.i 1596912
/proton/trunk/proton-c/bindings/ruby/ruby.i 1596912
/proton/trunk/proton-c/include/proton/buffer.h 1596912
/proton/trunk/proton-c/include/proton/types.h 1596912
/proton/trunk/proton-c/src/buffer.c 1596912
/proton/trunk/proton-c/src/codec/codec.c 1596912
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
/proton/trunk/proton-c/src/messenger/messenger.c 1596912
/proton/trunk/proton-c/src/sasl/sasl.c 1596912
/proton/trunk/proton-c/src/types.c 1596912
Diff: https://reviews.apache.org/r/21929/diff/
Testing
-------
ctest
Thanks,
Andrew Stitcher
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Darryl Pierce <dp...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/#review44010
-----------------------------------------------------------
Ship it!
- Darryl Pierce
On May 27, 2014, 4:32 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21929/
> -----------------------------------------------------------
>
> (Updated May 27, 2014, 4:32 p.m.)
>
>
> Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
>
>
> Bugs: PROTON-429
> https://issues.apache.org/jira/browse/PROTON-429
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
>
> Points/Questions:
>
> 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
>
> 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
>
> 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/perl/perl.i 1596912
> /proton/trunk/proton-c/bindings/php/php.i 1596912
> /proton/trunk/proton-c/bindings/python/python.i 1596912
> /proton/trunk/proton-c/bindings/ruby/ruby.i 1596912
> /proton/trunk/proton-c/include/proton/buffer.h 1596912
> /proton/trunk/proton-c/include/proton/types.h 1596912
> /proton/trunk/proton-c/src/buffer.c 1596912
> /proton/trunk/proton-c/src/codec/codec.c 1596912
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
> /proton/trunk/proton-c/src/messenger/messenger.c 1596912
> /proton/trunk/proton-c/src/sasl/sasl.c 1596912
> /proton/trunk/proton-c/src/types.c 1596912
>
> Diff: https://reviews.apache.org/r/21929/diff/
>
>
> Testing
> -------
>
> ctest
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/#review44138
-----------------------------------------------------------
/proton/trunk/proton-c/bindings/perl/perl.i
<https://reviews.apache.org/r/21929/#comment78463>
I believe there is a central location you can put %ignore pn_bytes_t rather than repeating it in all the different <lang>.i files. Look for cproton.i underneath include/proton
- Rafael Schloming
On May 27, 2014, 4:32 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21929/
> -----------------------------------------------------------
>
> (Updated May 27, 2014, 4:32 p.m.)
>
>
> Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
>
>
> Bugs: PROTON-429
> https://issues.apache.org/jira/browse/PROTON-429
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
>
> Points/Questions:
>
> 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
>
> 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
>
> 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/perl/perl.i 1596912
> /proton/trunk/proton-c/bindings/php/php.i 1596912
> /proton/trunk/proton-c/bindings/python/python.i 1596912
> /proton/trunk/proton-c/bindings/ruby/ruby.i 1596912
> /proton/trunk/proton-c/include/proton/buffer.h 1596912
> /proton/trunk/proton-c/include/proton/types.h 1596912
> /proton/trunk/proton-c/src/buffer.c 1596912
> /proton/trunk/proton-c/src/codec/codec.c 1596912
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
> /proton/trunk/proton-c/src/messenger/messenger.c 1596912
> /proton/trunk/proton-c/src/sasl/sasl.c 1596912
> /proton/trunk/proton-c/src/types.c 1596912
>
> Diff: https://reviews.apache.org/r/21929/diff/
>
>
> Testing
> -------
>
> ctest
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/#review44253
-----------------------------------------------------------
Ship it!
Modulo my naming comment I think this change is good to go.
- Rafael Schloming
On May 29, 2014, 4:58 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21929/
> -----------------------------------------------------------
>
> (Updated May 29, 2014, 4:58 a.m.)
>
>
> Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
>
>
> Bugs: PROTON-429
> https://issues.apache.org/jira/browse/PROTON-429
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
>
> Points/Questions:
>
> 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
>
> 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
>
> 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/include/proton/buffer.h 1596912
> /proton/trunk/proton-c/include/proton/cproton.i 1596912
> /proton/trunk/proton-c/include/proton/types.h 1596912
> /proton/trunk/proton-c/src/buffer.c 1596912
> /proton/trunk/proton-c/src/codec/codec.c 1596912
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
> /proton/trunk/proton-c/src/messenger/messenger.c 1596912
> /proton/trunk/proton-c/src/sasl/sasl.c 1596912
> /proton/trunk/proton-c/src/types.c 1596912
>
> Diff: https://reviews.apache.org/r/21929/diff/
>
>
> Testing
> -------
>
> ctest
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Andrew Stitcher <as...@apache.org>.
> On May 29, 2014, 10:52 a.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/include/proton/buffer.h, line 35
> > <https://reviews.apache.org/r/21929/diff/2/?file=598509#file598509line35>
> >
> > It occurs to me that the bytes/wbytes naming isn't super accurate. I think pn_bytes_t is fine because it is used to pass around actual bytes, however in the case of pn_wbytes_t it is really being used to pass around a chunk of memory which (at least initially) may hold no meaningful bytes.
> >
> > I'm thinking a different name might better reflect the usage and also make the split seem less ugly. Maybe pn_mem_t or something along those lines?
In the submitted change the struct is called pn_buffer_memory_t
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/#review44252
-----------------------------------------------------------
On May 29, 2014, 4:58 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21929/
> -----------------------------------------------------------
>
> (Updated May 29, 2014, 4:58 a.m.)
>
>
> Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
>
>
> Bugs: PROTON-429
> https://issues.apache.org/jira/browse/PROTON-429
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
>
> Points/Questions:
>
> 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
>
> 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
>
> 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/include/proton/buffer.h 1596912
> /proton/trunk/proton-c/include/proton/cproton.i 1596912
> /proton/trunk/proton-c/include/proton/types.h 1596912
> /proton/trunk/proton-c/src/buffer.c 1596912
> /proton/trunk/proton-c/src/codec/codec.c 1596912
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
> /proton/trunk/proton-c/src/messenger/messenger.c 1596912
> /proton/trunk/proton-c/src/sasl/sasl.c 1596912
> /proton/trunk/proton-c/src/types.c 1596912
>
> Diff: https://reviews.apache.org/r/21929/diff/
>
>
> Testing
> -------
>
> ctest
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/#review44252
-----------------------------------------------------------
/proton/trunk/proton-c/include/proton/buffer.h
<https://reviews.apache.org/r/21929/#comment78602>
It occurs to me that the bytes/wbytes naming isn't super accurate. I think pn_bytes_t is fine because it is used to pass around actual bytes, however in the case of pn_wbytes_t it is really being used to pass around a chunk of memory which (at least initially) may hold no meaningful bytes.
I'm thinking a different name might better reflect the usage and also make the split seem less ugly. Maybe pn_mem_t or something along those lines?
- Rafael Schloming
On May 29, 2014, 4:58 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21929/
> -----------------------------------------------------------
>
> (Updated May 29, 2014, 4:58 a.m.)
>
>
> Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
>
>
> Bugs: PROTON-429
> https://issues.apache.org/jira/browse/PROTON-429
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
>
> Points/Questions:
>
> 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
>
> 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
>
> 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/include/proton/buffer.h 1596912
> /proton/trunk/proton-c/include/proton/cproton.i 1596912
> /proton/trunk/proton-c/include/proton/types.h 1596912
> /proton/trunk/proton-c/src/buffer.c 1596912
> /proton/trunk/proton-c/src/codec/codec.c 1596912
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
> /proton/trunk/proton-c/src/messenger/messenger.c 1596912
> /proton/trunk/proton-c/src/sasl/sasl.c 1596912
> /proton/trunk/proton-c/src/types.c 1596912
>
> Diff: https://reviews.apache.org/r/21929/diff/
>
>
> Testing
> -------
>
> ctest
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/
-----------------------------------------------------------
(Updated May 29, 2014, 4:58 a.m.)
Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
Changes
-------
Updated change addressing Rafi's advice to centralise the %ignore pn_bytes_t in swig
Bugs: PROTON-429
https://issues.apache.org/jira/browse/PROTON-429
Repository: qpid
Description
-------
Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
Points/Questions:
1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
Diffs (updated)
-----
/proton/trunk/proton-c/include/proton/buffer.h 1596912
/proton/trunk/proton-c/include/proton/cproton.i 1596912
/proton/trunk/proton-c/include/proton/types.h 1596912
/proton/trunk/proton-c/src/buffer.c 1596912
/proton/trunk/proton-c/src/codec/codec.c 1596912
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
/proton/trunk/proton-c/src/messenger/messenger.c 1596912
/proton/trunk/proton-c/src/sasl/sasl.c 1596912
/proton/trunk/proton-c/src/types.c 1596912
Diff: https://reviews.apache.org/r/21929/diff/
Testing
-------
ctest
Thanks,
Andrew Stitcher
Re: Review Request 21929: Split apart the read-only and writable aspects of
pn_bytes_t
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21929/
-----------------------------------------------------------
(Updated May 27, 2014, 4:32 p.m.)
Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross.
Changes
-------
Added Ted as reviewer (as it is his original JIRA)
Bugs: PROTON-429
https://issues.apache.org/jira/browse/PROTON-429
Repository: qpid
Description
-------
Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API.
Points/Questions:
1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings.
2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added.
3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function.
Diffs
-----
/proton/trunk/proton-c/bindings/perl/perl.i 1596912
/proton/trunk/proton-c/bindings/php/php.i 1596912
/proton/trunk/proton-c/bindings/python/python.i 1596912
/proton/trunk/proton-c/bindings/ruby/ruby.i 1596912
/proton/trunk/proton-c/include/proton/buffer.h 1596912
/proton/trunk/proton-c/include/proton/types.h 1596912
/proton/trunk/proton-c/src/buffer.c 1596912
/proton/trunk/proton-c/src/codec/codec.c 1596912
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912
/proton/trunk/proton-c/src/messenger/messenger.c 1596912
/proton/trunk/proton-c/src/sasl/sasl.c 1596912
/proton/trunk/proton-c/src/types.c 1596912
Diff: https://reviews.apache.org/r/21929/diff/
Testing
-------
ctest
Thanks,
Andrew Stitcher