You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2016/06/27 15:15:41 UTC
Review Request 49266: NO-JIRA: Add value_ref,
clean access to pn_data_t* via value interface
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49266/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Cliff Jansen.
Repository: qpid-proton-git
Description
-------
value_ref allows a class to return a "value&" that provides access to an
internally-held pn_data_t*, and provides a single point of "friendship" for
creating a proton::value by copying from a value_ref.
NOTE: Internal only, no API change.
Diffs
-----
proton-c/bindings/cpp/include/proton/internal/config.hpp cd5370e741f04793c14e3069b7daa27fb93044bd
proton-c/bindings/cpp/include/proton/message.hpp 4a562f7b4e3991c12d896ac466feee5e2573e938
proton-c/bindings/cpp/include/proton/value.hpp 5438e00b0145c5a81bbe37aad94b0cde9b727053
proton-c/bindings/cpp/src/decoder.cpp b215539c532b66cba90c96adf82df370d1d99859
proton-c/bindings/cpp/src/encoder.cpp 79f7826050f5e54e632c71529607ee013e29975b
proton-c/bindings/cpp/src/error_condition.cpp f200010b843ef9e9a85723e1378d28107712660c
proton-c/bindings/cpp/src/message.cpp 6a05d90b0be5fc2e250bd20bc6bf46f4186b2a09
proton-c/bindings/cpp/src/proton_bits.cpp 5514ebc6b0a974284d1633ba95b1d6ad6a79fdb0
proton-c/bindings/cpp/src/terminus.cpp f378fdbe395da00a7e68a65ca96ba8a893ae5fea
proton-c/bindings/cpp/src/value.cpp d0ef2ee83ade61d754af992d46345171d035d75d
Diff: https://reviews.apache.org/r/49266/diff/
Testing
-------
ctest -R cpp on gcc/clang X 03/11 + VS 12
Thanks,
Alan Conway
Re: Review Request 49266: NO-JIRA: Add value_ref,
clean access to pn_data_t* via value interface
Posted by Alan Conway <ac...@redhat.com>.
> On June 29, 2016, 2:37 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/src/message.cpp, line 57
> > <https://reviews.apache.org/r/49266/diff/1/?file=1431086#file1431086line57>
> >
> > Perhaps there should be explicit release() member. Then the comment would be redundant.
> >
> > Perhaps the conventional smart_ptr release() might actually be useful in this case too - I'm not exactly sure why, but it's a tried and tested technique.
Like all the proton wrappers, value_ref does not have pointer operators or any public relationship with a pointee type so there is nothing that release() can return, at least not in the usual smart-pointer sense. How about reset()?
> On June 29, 2016, 2:37 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/internal/config.hpp, line 86
> > <https://reviews.apache.org/r/49266/diff/1/?file=1431080#file1431080line86>
> >
> > I can't see where you've used this - if you didn't can we get rid of it?
Dropped. I didn't use it in the end.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49266/#review139991
-----------------------------------------------------------
On June 27, 2016, 3:15 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49266/
> -----------------------------------------------------------
>
> (Updated June 27, 2016, 3:15 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Cliff Jansen.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> value_ref allows a class to return a "value&" that provides access to an
> internally-held pn_data_t*, and provides a single point of "friendship" for
> creating a proton::value by copying from a value_ref.
>
> NOTE: Internal only, no API change.
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/internal/config.hpp cd5370e741f04793c14e3069b7daa27fb93044bd
> proton-c/bindings/cpp/include/proton/message.hpp 4a562f7b4e3991c12d896ac466feee5e2573e938
> proton-c/bindings/cpp/include/proton/value.hpp 5438e00b0145c5a81bbe37aad94b0cde9b727053
> proton-c/bindings/cpp/src/decoder.cpp b215539c532b66cba90c96adf82df370d1d99859
> proton-c/bindings/cpp/src/encoder.cpp 79f7826050f5e54e632c71529607ee013e29975b
> proton-c/bindings/cpp/src/error_condition.cpp f200010b843ef9e9a85723e1378d28107712660c
> proton-c/bindings/cpp/src/message.cpp 6a05d90b0be5fc2e250bd20bc6bf46f4186b2a09
> proton-c/bindings/cpp/src/proton_bits.cpp 5514ebc6b0a974284d1633ba95b1d6ad6a79fdb0
> proton-c/bindings/cpp/src/terminus.cpp f378fdbe395da00a7e68a65ca96ba8a893ae5fea
> proton-c/bindings/cpp/src/value.cpp d0ef2ee83ade61d754af992d46345171d035d75d
>
> Diff: https://reviews.apache.org/r/49266/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on gcc/clang X 03/11 + VS 12
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 49266: NO-JIRA: Add value_ref,
clean access to pn_data_t* via value interface
Posted by Andrew Stitcher <as...@apache.org>.
> On June 29, 2016, 2:37 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/src/message.cpp, line 57
> > <https://reviews.apache.org/r/49266/diff/1/?file=1431086#file1431086line57>
> >
> > Perhaps there should be explicit release() member. Then the comment would be redundant.
> >
> > Perhaps the conventional smart_ptr release() might actually be useful in this case too - I'm not exactly sure why, but it's a tried and tested technique.
>
> Alan Conway wrote:
> Like all the proton wrappers, value_ref does not have pointer operators or any public relationship with a pointee type so there is nothing that release() can return, at least not in the usual smart-pointer sense. How about reset()?
oh yes, good point - reset() seems a good name.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49266/#review139991
-----------------------------------------------------------
On June 27, 2016, 3:15 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49266/
> -----------------------------------------------------------
>
> (Updated June 27, 2016, 3:15 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Cliff Jansen.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> value_ref allows a class to return a "value&" that provides access to an
> internally-held pn_data_t*, and provides a single point of "friendship" for
> creating a proton::value by copying from a value_ref.
>
> NOTE: Internal only, no API change.
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/internal/config.hpp cd5370e741f04793c14e3069b7daa27fb93044bd
> proton-c/bindings/cpp/include/proton/message.hpp 4a562f7b4e3991c12d896ac466feee5e2573e938
> proton-c/bindings/cpp/include/proton/value.hpp 5438e00b0145c5a81bbe37aad94b0cde9b727053
> proton-c/bindings/cpp/src/decoder.cpp b215539c532b66cba90c96adf82df370d1d99859
> proton-c/bindings/cpp/src/encoder.cpp 79f7826050f5e54e632c71529607ee013e29975b
> proton-c/bindings/cpp/src/error_condition.cpp f200010b843ef9e9a85723e1378d28107712660c
> proton-c/bindings/cpp/src/message.cpp 6a05d90b0be5fc2e250bd20bc6bf46f4186b2a09
> proton-c/bindings/cpp/src/proton_bits.cpp 5514ebc6b0a974284d1633ba95b1d6ad6a79fdb0
> proton-c/bindings/cpp/src/terminus.cpp f378fdbe395da00a7e68a65ca96ba8a893ae5fea
> proton-c/bindings/cpp/src/value.cpp d0ef2ee83ade61d754af992d46345171d035d75d
>
> Diff: https://reviews.apache.org/r/49266/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on gcc/clang X 03/11 + VS 12
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 49266: NO-JIRA: Add value_ref,
clean access to pn_data_t* via value interface
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49266/#review139991
-----------------------------------------------------------
I like this a lot (subject of course to usual carping!)
proton-c/bindings/cpp/include/proton/internal/config.hpp (line 86)
<https://reviews.apache.org/r/49266/#comment205334>
I can't see where you've used this - if you didn't can we get rid of it?
proton-c/bindings/cpp/src/message.cpp (line 57)
<https://reviews.apache.org/r/49266/#comment205331>
Perhaps there should be explicit release() member. Then the comment would be redundant.
Perhaps the conventional smart_ptr release() might actually be useful in this case too - I'm not exactly sure why, but it's a tried and tested technique.
- Andrew Stitcher
On June 27, 2016, 3:15 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49266/
> -----------------------------------------------------------
>
> (Updated June 27, 2016, 3:15 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Cliff Jansen.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> value_ref allows a class to return a "value&" that provides access to an
> internally-held pn_data_t*, and provides a single point of "friendship" for
> creating a proton::value by copying from a value_ref.
>
> NOTE: Internal only, no API change.
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/internal/config.hpp cd5370e741f04793c14e3069b7daa27fb93044bd
> proton-c/bindings/cpp/include/proton/message.hpp 4a562f7b4e3991c12d896ac466feee5e2573e938
> proton-c/bindings/cpp/include/proton/value.hpp 5438e00b0145c5a81bbe37aad94b0cde9b727053
> proton-c/bindings/cpp/src/decoder.cpp b215539c532b66cba90c96adf82df370d1d99859
> proton-c/bindings/cpp/src/encoder.cpp 79f7826050f5e54e632c71529607ee013e29975b
> proton-c/bindings/cpp/src/error_condition.cpp f200010b843ef9e9a85723e1378d28107712660c
> proton-c/bindings/cpp/src/message.cpp 6a05d90b0be5fc2e250bd20bc6bf46f4186b2a09
> proton-c/bindings/cpp/src/proton_bits.cpp 5514ebc6b0a974284d1633ba95b1d6ad6a79fdb0
> proton-c/bindings/cpp/src/terminus.cpp f378fdbe395da00a7e68a65ca96ba8a893ae5fea
> proton-c/bindings/cpp/src/value.cpp d0ef2ee83ade61d754af992d46345171d035d75d
>
> Diff: https://reviews.apache.org/r/49266/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on gcc/clang X 03/11 + VS 12
>
>
> Thanks,
>
> Alan Conway
>
>