You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/05/24 18:45:29 UTC
[GitHub] [qpid-proton] jiridanek opened a new pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
jiridanek opened a new pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r638197366
##########
File path: c/tests/data_test.cpp
##########
@@ -124,6 +124,7 @@ static void check_encode_decode(auto_free<pn_data_t, pn_data_free>& src) {
// Decode buf to data
int dec_size = pn_data_decode(data, buf, BUFSIZE - 1);
pn_error_t *dec_err = pn_data_error(data);
+ CHECK(pn_error_code(dec_err) == 0);
Review comment:
Unused variable error, there is multiple cases like this. I am surprised that compiler did not error on this before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639139966
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
Ok, I edited my comment too late, now the conversation does not make sense. Revert!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639137399
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
This class is something like an `std::unique_ptr` with a deleter. Apparently _not_ having explicit `operator*` was considered and dismissed, for reasons https://herbsutter.com/2012/06/21/reader-qa-why-dont-modern-smart-pointers-implicitly-convert-to/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639136765
##########
File path: c/tests/pn_test.hpp
##########
@@ -129,7 +130,7 @@ struct handler {
struct driver : public ::pn_connection_driver_t {
struct handler &handler;
- driver(struct handler &h);
+ explicit driver(struct handler &h);
Review comment:
Ah, I'd forgotten the new iinitialisation syntax thing - that makes this much more compelling as a rule. It's definitely the better default anyway - but also makes conversion operators safer (even if it would make for widespread changes!).
Making this change to the exported include files would change the API, so we can't do it easily there (but actually not the ABI I think)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639133888
##########
File path: c/tests/pn_test.hpp
##########
@@ -129,7 +130,7 @@ struct handler {
struct driver : public ::pn_connection_driver_t {
struct handler &handler;
- driver(struct handler &h);
+ explicit driver(struct handler &h);
Review comment:
I got it from clang-tidy and https://google.github.io/styleguide/cppguide.html#Implicit_Conversions. The widely accepted rule seems to put `explicit` everywhere, unless there is a very good reason not to. (And I can do this freely here, because I am not touching public API.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639141389
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
Honestly I'm fine with putting this PR in as is and then moving it closer to good C++11 piece-by-piece
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639002890
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
Review comment:
I don't think that explicit does anything useful here (not harmful though). The major (only?) reason to use explicit is to stop automatic conversions from integer types happening for templated types - this constructor takes a pointer so there is no chance of that happening - There's no harm in making this explicit though.
##########
File path: c/tests/pn_test.hpp
##########
@@ -129,7 +130,7 @@ struct handler {
struct driver : public ::pn_connection_driver_t {
struct handler &handler;
- driver(struct handler &h);
+ explicit driver(struct handler &h);
Review comment:
As above I don't think this actually does anything, but it's not harmful at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639138782
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
Actually it could be replaced with unique_ptr now - this version will work with C++03 which isn't needed any more. [std::unique_ptr has an optional deleter capability]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639139343
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
Nut not having explicit here is the opposite of the advice from Herb Sutter I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#discussion_r639137399
##########
File path: c/tests/pn_test.hpp
##########
@@ -46,13 +46,14 @@ namespace pn_test {
template <class T, void (*Free)(T *)> class auto_free {
T *ptr_;
auto_free &operator=(auto_free &x);
- auto_free(auto_free &x);
+public:
+ auto_free(auto_free &x) = delete;
public:
- auto_free(T *p = 0) : ptr_(p) {}
+ explicit auto_free(T *p = 0) : ptr_(p) {}
~auto_free() { Free(ptr_); }
T *get() const { return ptr_; }
- operator T *() const { return ptr_; }
+ operator T *() const { return ptr_; } // not marking explicit for convenience
Review comment:
This class is something like an `std::unique_ptr` with a deleter. Apparently having explicit `operator*` was considered and dismissed, for reasons https://herbsutter.com/2012/06/21/reader-qa-why-dont-modern-smart-pointers-implicitly-convert-to/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek merged pull request #315: PROTON-2389 Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek merged pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on pull request #315: NO-LINKED-JIRA-YET Update Catch to Catch2
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #315:
URL: https://github.com/apache/qpid-proton/pull/315#issuecomment-847981177
@astitcher I wanted to have another look at this. i committed this right before going to sleep, actually. One thing that I'd want to understand are the previously unused variables. How come the tests compiled before just fine?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org