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/08/23 15:58:41 UTC
Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/
-----------------------------------------------------------
Review request for qpid and Andrew Stitcher.
Repository: qpid-proton-git
Description
-------
Added empty() to scalar_base()
Moved internal::scalar_base to proton::scalar_base. All it's public member
functions are part of the public API for proton::scalar, message_id and annotation_key
so putting the class that contains them in the namespace internal makes that unclear
and prevents doxygen from documenting them.
Moved internal::value_base::empty() and type() to proton::value. There's no need for them
to be on the internal class and they are public API.
WIP: make scalar_base public, add empty() to scalar base.
Note all types need empty()
Diffs
-----
proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
Diff: https://reviews.apache.org/r/51336/diff/
Testing
-------
ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
Thanks,
Alan Conway
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/
-----------------------------------------------------------
(Updated Aug. 24, 2016, 3:33 p.m.)
Review request for qpid and Andrew Stitcher.
Changes
-------
added a unit test
Repository: qpid-proton-git
Description
-------
Added empty() to scalar_base()
Moved internal::scalar_base to proton::scalar_base. All it's public member
functions are part of the public API for proton::scalar, message_id and annotation_key
so putting the class that contains them in the namespace internal makes that unclear
and prevents doxygen from documenting them.
Moved internal::value_base::empty() and type() to proton::value. There's no need for them
to be on the internal class and they are public API.
Note all scalar types need empty() because although the restricted types are
only allowed to be set as specific typesm, they can be missing from a message
which is represented by empty().
Diffs (updated)
-----
proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
Diff: https://reviews.apache.org/r/51336/diff/
Testing
-------
ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
Thanks,
Alan Conway
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/
-----------------------------------------------------------
(Updated Aug. 24, 2016, 2:51 p.m.)
Review request for qpid and Andrew Stitcher.
Changes
-------
missing extern, will I ever learn?
Repository: qpid-proton-git
Description
-------
Added empty() to scalar_base()
Moved internal::scalar_base to proton::scalar_base. All it's public member
functions are part of the public API for proton::scalar, message_id and annotation_key
so putting the class that contains them in the namespace internal makes that unclear
and prevents doxygen from documenting them.
Moved internal::value_base::empty() and type() to proton::value. There's no need for them
to be on the internal class and they are public API.
Note all scalar types need empty() because although the restricted types are
only allowed to be set as specific typesm, they can be missing from a message
which is represented by empty().
Diffs (updated)
-----
proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
Diff: https://reviews.apache.org/r/51336/diff/
Testing
-------
ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
Thanks,
Alan Conway
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
>
> Andrew Stitcher wrote:
> Now we're really getting to quibles!
>
> I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
Now we are just agreeing in an argumentative way.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 2:51 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 2:51 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Gordon Sim <gs...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
>
> Andrew Stitcher wrote:
> Now we're really getting to quibles!
>
> I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
>
> Alan Conway wrote:
> Now we are just agreeing in an argumentative way.
Just to join in the pedantry, both message_id and correlation_id *can* be null. The properties section is a list, and a null entry in that list is perfectly valid (indeed would be required e.g. if you didn't want to provide a correlation-id value but did want to provide a content-type).
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > I'm not 100% convinced that moving scalar_base out of internal is justified by the inability to get doxygen to generate the correct docs. Can't we fix doxygen somehow?
> >
> > Why scalar_base but not value_base? they have the same function in the API structure.
> >
> > I think it may be correct to actually pull in avarything from the base classes with a using directive anyway to give the correct name lookup semantics (it matters if you have overloading).
value_base doesn't have the same function, it's solving a template type recursion problem and there was no need for it to have any public functions so I just moved them to value.
scalar_base is providing common functionality for scalar, messsage_id etc. and is used by the encoder and decoder to do the common encoding for them all.
I dithered myself. I decided not to "fix" doxygen because doxygen is accurately reflecting our code conventions: If namespace internal means "don't touch that" then having public API functions inherited from an internal base class is a mixed message at best. Other options, which I can implement if you prefer:
1. make a separate public proton::scalar_base with just those two functions that delegates to the internal one
2. drop the inheritance and duplicate the public functions on all the derived classes, delegate to an internal impl object.
2. is cleanest, 1. less work. I'll bite the bullet and do 2. if you feel that's the right thing
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Andrew Stitcher <as...@apache.org>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
Now we're really getting to quibles!
I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Andrew Stitcher <as...@apache.org>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
In which case this is a bug fix.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > I'm not 100% convinced that moving scalar_base out of internal is justified by the inability to get doxygen to generate the correct docs. Can't we fix doxygen somehow?
> >
> > Why scalar_base but not value_base? they have the same function in the API structure.
> >
> > I think it may be correct to actually pull in avarything from the base classes with a using directive anyway to give the correct name lookup semantics (it matters if you have overloading).
>
> Alan Conway wrote:
> value_base doesn't have the same function, it's solving a template type recursion problem and there was no need for it to have any public functions so I just moved them to value.
> scalar_base is providing common functionality for scalar, messsage_id etc. and is used by the encoder and decoder to do the common encoding for them all.
>
> I dithered myself. I decided not to "fix" doxygen because doxygen is accurately reflecting our code conventions: If namespace internal means "don't touch that" then having public API functions inherited from an internal base class is a mixed message at best. Other options, which I can implement if you prefer:
> 1. make a separate public proton::scalar_base with just those two functions that delegates to the internal one
> 2. drop the inheritance and duplicate the public functions on all the derived classes, delegate to an internal impl object.
>
> 2. is cleanest, 1. less work. I'll bite the bullet and do 2. if you feel that's the right thing
>
> Andrew Stitcher wrote:
> I don't think internal exactly means "don't touch that", rather you'll never get this directly or you'll never deal with this object only one of it's children or if you get one of these don't look inside just rely on it's interface.
>
> So it's not that they don't provide API, but that the API is thought of as attaching to the child - that is why I suggested using "using" as that gives the expected namespace lookup if there are (accidentally) overloads with the same name in parent and child.
>
> So I'd disagree about doxygen respecting our code conventions, but I'm beginning to suspect that we think "internal" means different things.
>
> Alan Conway wrote:
> To me, the public API is everything you have to document so the user can use the API. Internal is everything else. It's not just doxygen, someone trying to read the headers will most likely never follow the internal::scalar_base inheritance to discover the hidden public API because we called it "internal::scalar_base". So we need something in a public header that makes it clear that this is public stuff, but I can have a go at how doxygen documents using statements or just add an inline function in each subclass.
I can't do it. I don't understand how a public base class, with public functions, all intended to be called by the user, is "internal" or "detail" in any sense. This is normal is-a inheritance. C++ doesn't have type restrictions (we can't say `message_id : restriction_of scalar` and remove functions we don't want) so a common base class is the only way to do it. Why would we want to hide that commonality in internal:: or detail:: ?
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Gordon Sim <gs...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
>
> Andrew Stitcher wrote:
> Now we're really getting to quibles!
>
> I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
>
> Alan Conway wrote:
> Now we are just agreeing in an argumentative way.
>
> Gordon Sim wrote:
> Just to join in the pedantry, both message_id and correlation_id *can* be null. The properties section is a list, and a null entry in that list is perfectly valid (indeed would be required e.g. if you didn't want to provide a correlation-id value but did want to provide a content-type).
>
> Alan Conway wrote:
> OMG. So we have 3 states: 1. not present in props list (list truncated), 2. present but null in props list (because we provide values later in the list or have a list with trailing NULLs) and 3. present with an actual value. However I don't think there's any not-entirely-insane reading of the AMQP spec where 1. and 2. are not semantically equivalent, and anyway proton-C doesn't make the distinction. So lets pretend Gordon didn't raise that point.
To be clear, I'm just saying I don't see a distinction between null and not there. An optional field may not be there (and that may be indicated on the wire by an explicit null or implied by a truncated list).
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > I'm not 100% convinced that moving scalar_base out of internal is justified by the inability to get doxygen to generate the correct docs. Can't we fix doxygen somehow?
> >
> > Why scalar_base but not value_base? they have the same function in the API structure.
> >
> > I think it may be correct to actually pull in avarything from the base classes with a using directive anyway to give the correct name lookup semantics (it matters if you have overloading).
>
> Alan Conway wrote:
> value_base doesn't have the same function, it's solving a template type recursion problem and there was no need for it to have any public functions so I just moved them to value.
> scalar_base is providing common functionality for scalar, messsage_id etc. and is used by the encoder and decoder to do the common encoding for them all.
>
> I dithered myself. I decided not to "fix" doxygen because doxygen is accurately reflecting our code conventions: If namespace internal means "don't touch that" then having public API functions inherited from an internal base class is a mixed message at best. Other options, which I can implement if you prefer:
> 1. make a separate public proton::scalar_base with just those two functions that delegates to the internal one
> 2. drop the inheritance and duplicate the public functions on all the derived classes, delegate to an internal impl object.
>
> 2. is cleanest, 1. less work. I'll bite the bullet and do 2. if you feel that's the right thing
>
> Andrew Stitcher wrote:
> I don't think internal exactly means "don't touch that", rather you'll never get this directly or you'll never deal with this object only one of it's children or if you get one of these don't look inside just rely on it's interface.
>
> So it's not that they don't provide API, but that the API is thought of as attaching to the child - that is why I suggested using "using" as that gives the expected namespace lookup if there are (accidentally) overloads with the same name in parent and child.
>
> So I'd disagree about doxygen respecting our code conventions, but I'm beginning to suspect that we think "internal" means different things.
To me, the public API is everything you have to document so the user can use the API. Internal is everything else. It's not just doxygen, someone trying to read the headers will most likely never follow the internal::scalar_base inheritance to discover the hidden public API because we called it "internal::scalar_base". So we need something in a public header that makes it clear that this is public stuff, but I can have a go at how doxygen documents using statements or just add an inline function in each subclass.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
>
> Andrew Stitcher wrote:
> Now we're really getting to quibles!
>
> I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
>
> Alan Conway wrote:
> Now we are just agreeing in an argumentative way.
>
> Gordon Sim wrote:
> Just to join in the pedantry, both message_id and correlation_id *can* be null. The properties section is a list, and a null entry in that list is perfectly valid (indeed would be required e.g. if you didn't want to provide a correlation-id value but did want to provide a content-type).
>
> Alan Conway wrote:
> OMG. So we have 3 states: 1. not present in props list (list truncated), 2. present but null in props list (because we provide values later in the list or have a list with trailing NULLs) and 3. present with an actual value. However I don't think there's any not-entirely-insane reading of the AMQP spec where 1. and 2. are not semantically equivalent, and anyway proton-C doesn't make the distinction. So lets pretend Gordon didn't raise that point.
>
> Gordon Sim wrote:
> To be clear, I'm just saying I don't see a distinction between null and not there. An optional field may not be there (and that may be indicated on the wire by an explicit null or implied by a truncated list).
I think we are now all agreeing both pedantically and argumentatively. We have the verbose agreement spectrum covered.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45
> > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45>
> >
> > This is an actual API change (even if it is a correct one).
> >
> > Can message_id really be empty?
>
> Alan Conway wrote:
> No, but it can be missing from the message properties, which is different from being present with 0 or "". If it is missing there's no safe/sensible way to choose a default "empty" value - should it be an empty string or binary, or a 0 integer? Also it is quite valid not to set a correlation_id, which is semantically different from a correlation-id of 0 or "", so we do need to represent "missing value" somehow.
>
> Andrew Stitcher wrote:
> Ok, I think you actually answered that message_id *can* be empty! It's empty if correlation_id is not set at all.
>
> In which case this is a bug fix.
>
> Alan Conway wrote:
> Well technically: message_id can only be one of 4 types, NULL is not one of them. So a message_id cannot be empty, which is why I didn't have it there originally. However it can be *missing* from a message, so adding the "empty" possibility allows us to model that in a natural way which is similar to how C, python etc. model it with empty pn_data_t, NULL, None etc.
>
> Andrew Stitcher wrote:
> Now we're really getting to quibles!
>
> I'd say that a correlation_id can't be null (but there might not be one at all) but the type modelling it message_id can be null to model that it isn't present!
>
> Alan Conway wrote:
> Now we are just agreeing in an argumentative way.
>
> Gordon Sim wrote:
> Just to join in the pedantry, both message_id and correlation_id *can* be null. The properties section is a list, and a null entry in that list is perfectly valid (indeed would be required e.g. if you didn't want to provide a correlation-id value but did want to provide a content-type).
OMG. So we have 3 states: 1. not present in props list (list truncated), 2. present but null in props list (because we provide values later in the list or have a list with trailing NULLs) and 3. present with an actual value. However I don't think there's any not-entirely-insane reading of the AMQP spec where 1. and 2. are not semantically equivalent, and anyway proton-C doesn't make the distinction. So lets pretend Gordon didn't raise that point.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Andrew Stitcher <as...@apache.org>.
> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote:
> > I'm not 100% convinced that moving scalar_base out of internal is justified by the inability to get doxygen to generate the correct docs. Can't we fix doxygen somehow?
> >
> > Why scalar_base but not value_base? they have the same function in the API structure.
> >
> > I think it may be correct to actually pull in avarything from the base classes with a using directive anyway to give the correct name lookup semantics (it matters if you have overloading).
>
> Alan Conway wrote:
> value_base doesn't have the same function, it's solving a template type recursion problem and there was no need for it to have any public functions so I just moved them to value.
> scalar_base is providing common functionality for scalar, messsage_id etc. and is used by the encoder and decoder to do the common encoding for them all.
>
> I dithered myself. I decided not to "fix" doxygen because doxygen is accurately reflecting our code conventions: If namespace internal means "don't touch that" then having public API functions inherited from an internal base class is a mixed message at best. Other options, which I can implement if you prefer:
> 1. make a separate public proton::scalar_base with just those two functions that delegates to the internal one
> 2. drop the inheritance and duplicate the public functions on all the derived classes, delegate to an internal impl object.
>
> 2. is cleanest, 1. less work. I'll bite the bullet and do 2. if you feel that's the right thing
I don't think internal exactly means "don't touch that", rather you'll never get this directly or you'll never deal with this object only one of it's children or if you get one of these don't look inside just rely on it's interface.
So it's not that they don't provide API, but that the API is thought of as attaching to the child - that is why I suggested using "using" as that gives the expected namespace lookup if there are (accidentally) overloads with the same name in parent and child.
So I'd disagree about doxygen respecting our code conventions, but I'm beginning to suspect that we think "internal" means different things.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 3:33 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message.hpp 3f683f655346293ab6c043c56d9caed47a391b85
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/message.cpp ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945
> proton-c/bindings/cpp/src/message_test.cpp 68205c9a04fe6e1edb2000bcc9091ef02748878f
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/#review146545
-----------------------------------------------------------
I'm not 100% convinced that moving scalar_base out of internal is justified by the inability to get doxygen to generate the correct docs. Can't we fix doxygen somehow?
Why scalar_base but not value_base? they have the same function in the API structure.
I think it may be correct to actually pull in avarything from the base classes with a using directive anyway to give the correct name lookup semantics (it matters if you have overloading).
proton-c/bindings/cpp/include/proton/message_id.hpp (line 45)
<https://reviews.apache.org/r/51336/#comment213050>
This is an actual API change (even if it is a correct one).
Can message_id really be empty?
- Andrew Stitcher
On Aug. 23, 2016, 4:02 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51336/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2016, 4:02 p.m.)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Added empty() to scalar_base()
>
> Moved internal::scalar_base to proton::scalar_base. All it's public member
> functions are part of the public API for proton::scalar, message_id and annotation_key
> so putting the class that contains them in the namespace internal makes that unclear
> and prevents doxygen from documenting them.
>
> Moved internal::value_base::empty() and type() to proton::value. There's no need for them
> to be on the internal class and they are public API.
>
> Note all scalar types need empty() because although the restricted types are
> only allowed to be set as specific typesm, they can be missing from a message
> which is represented by empty().
>
>
> Diffs
> -----
>
> proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
> proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
> proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
> proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
> proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
> proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
> proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
> proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
> proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
>
> Diff: https://reviews.apache.org/r/51336/diff/
>
>
> Testing
> -------
>
> ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 51336: PROTON-1287: c++: missing empty() method on
message_id, annotation_key
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51336/
-----------------------------------------------------------
(Updated Aug. 23, 2016, 4:02 p.m.)
Review request for qpid and Andrew Stitcher.
Changes
-------
Removed scalar::empty() as it is now inherited from scalar_base
Repository: qpid-proton-git
Description (updated)
-------
Added empty() to scalar_base()
Moved internal::scalar_base to proton::scalar_base. All it's public member
functions are part of the public API for proton::scalar, message_id and annotation_key
so putting the class that contains them in the namespace internal makes that unclear
and prevents doxygen from documenting them.
Moved internal::value_base::empty() and type() to proton::value. There's no need for them
to be on the internal class and they are public API.
Note all scalar types need empty() because although the restricted types are
only allowed to be set as specific typesm, they can be missing from a message
which is represented by empty().
Diffs (updated)
-----
proton-c/bindings/cpp/include/proton/annotation_key.hpp c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3
proton-c/bindings/cpp/include/proton/codec/encoder.hpp 847e0a9f29aba973f96fa06e3268a1bbe063cd53
proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp 2e491d193d604d52b7ec91af510770fc9b4ac66e
proton-c/bindings/cpp/include/proton/message_id.hpp 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9
proton-c/bindings/cpp/include/proton/scalar.hpp ab7cb2a514d697c365cf568b4be7a842ae29aa3e
proton-c/bindings/cpp/include/proton/value.hpp e7e9f7933038c1ba23b5afdbb102d59208e9ea4c
proton-c/bindings/cpp/src/encoder.cpp e718c402a46d5423b70d178c1903b6ad767eeec5
proton-c/bindings/cpp/src/scalar_base.cpp c005122dedcfabe135bb62fe6c16eafe4da630cb
proton-c/bindings/cpp/src/value.cpp c2232431f9f093e3530342ea5e3680a62737a4c8
Diff: https://reviews.apache.org/r/51336/diff/
Testing
-------
ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis
Thanks,
Alan Conway