You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/07/11 02:23:13 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request #6996: Make the setting of the continuation handler safer.
ywkaras opened a new pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996
These three code examples only differ in the first two lines:
https://godbolt.org/z/Gq38j7
https://godbolt.org/z/hGb9PT
https://godbolt.org/z/6MdKjn
They illustrate how using a C-style cast to convert a derived
class member function pointer to a base class member function
pointer bypasses a compile time check that is derived class is
actually derived from the base class.
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454027905
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
~~hmm, we have a version check of `__GNUC__` in InkAPITest.cc. Is this broken?
https://github.com/apache/trafficserver/blob/89b6b91d43eb43d16038ef8e9f9a9a1cfd2ed581/src/traffic_server/InkAPITest.cc#L25~~
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454028912
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
This looks standard approach
```
/* Test for GCC > 3.2.0 */
#if __GNUC__ > 3 || \
(__GNUC__ == 3 && (__GNUC_MINOR__ > 2 || \
(__GNUC_MINOR__ == 2 && \
__GNUC_PATCHLEVEL__ > 0))
```
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453377616
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,26 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
Review comment:
Can we check this at compile time & avoid casting twice like this?
```
template <class C, typename T>
constexpr ContinuationHandler
continuation_handler_void_ptr(int (C::*fp)(int, T *))
{
static_assert(std::is_base_of<Continuation, C> && std::is_convertible<C, Continuation>);
return reinterpret_cast<fp>;
}
```
BTW, we can check this clearly with the c++20 concept
```
template <class C, typename T>
requires std::derived_from(C, Continuation)
constexpr ContinuationHandler
continuation_handler_void_ptr(int (C::*fp)(int, T *))
{
```
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454027905
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
hmm, we have a version check of `__GNUC__` in InkAPITest.cc. Is this broken?
https://github.com/apache/trafficserver/blob/89b6b91d43eb43d16038ef8e9f9a9a1cfd2ed581/src/traffic_server/InkAPITest.cc#L25
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras closed pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454368819
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
`#if defined(__GNUC__) && (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 3)` does work for clang: https://godbolt.org/z/bGEz9P
But I'm not sure the Standard guarantees it will work for a different compiler that we may want to use in the future.
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453993086
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
We can use `&&` like below?
```
#if defined(__GNUC__) && (__GNUC__ > 7)
```
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#issuecomment-657616872
[approve ci Ubuntu]
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454686210
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Talked on slack, this is a workaround fix to avoid a bug of gcc-7 with static_assert. Let's remove when we drop the gcc-7 support.
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454033437
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Oh, I got point we need `static_assert` if `__GNUC__` is not defined for clang.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras merged pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras merged pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453991555
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,26 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
Review comment:
Ah, I missed the `constexpr` part. Great :+1:
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras removed a comment on pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#issuecomment-657616872
[approve ci Ubuntu]
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454001638
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Note that this does not compile: https://godbolt.org/z/erGjoj
I'm not sure the Standard guarantees that a compiler won't warn that `__GNUC__` is not defined when it is compared to 7.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453661783
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,26 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
Review comment:
It is a constexpr function, it will be evaluated at compile time, and cause no additional instructions in the executable.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453716199
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,26 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
Review comment:
Also, your alternative does not check that the handler function has the right parameters (an int and a pointer).
----------------------------------------------------------------
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
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454038877
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Another approach to avoid these preprocessors is adding a message as the second argument to use c++11 version of `static_assert(bool_constexpr , message)` .
> message can be omitted. (since C++17)
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454001638
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Note that this does not compile: https://godbolt.org/z/E5c3v6
I'm not sure the Standard guarantees that a compiler won't warn that __GNUC__ is not defined when it compared to 7.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454362651
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
That doesn't seem to work: https://godbolt.org/z/17cfTo
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454001638
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Note that this does not compile: https://godbolt.org/z/erGjoj
I'm not sure the Standard guarantees that a compiler won't warn that `__GNUC__` is not defined when it compared to 7.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r453716199
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,26 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
Review comment:
Also, your alternative does not check that the handler function has the right parameters (an int and a pointer).
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454001638
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Note that this does not compile: https://godbolt.org/z/erGjoj
I'm not sure the Standard guarantees that a compiler won't warn that __GNUC__ is not defined when it compared to 7.
----------------------------------------------------------------
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
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6996: Make the setting of the continuation handler safer.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6996:
URL: https://github.com/apache/trafficserver/pull/6996#discussion_r454001638
##########
File path: iocore/eventsystem/I_Continuation.h
##########
@@ -63,6 +63,35 @@ extern EThread *this_event_thread();
typedef int (Continuation::*ContinuationHandler)(int event, void *data);
+// Convert event handler pointer fp to type ContinuationHandler, but with a compiler error if class C is not
+// derived from the class Continuation.
+//
+template <class C, typename T>
+constexpr ContinuationHandler
+continuation_handler_void_ptr(int (C::*fp)(int, T *))
+{
+ auto fp2 = reinterpret_cast<int (C::*)(int, void *)>(fp);
+ return static_cast<ContinuationHandler>(fp2);
+}
+
+// Overload for nullptr.
+//
+constexpr ContinuationHandler continuation_handler_void_ptr(std::nullptr_t)
+{
+#undef X
+#if !defined(__GNUC__)
+#define X 1
+#else
+#define X (__GNUC__ > 7)
Review comment:
Note that this does not compile: https://godbolt.org/z/8cq3sd
I'm not sure the Standard guarantees that a compiler won't warn that __GNUC__ is not defined when it compared to 7.
----------------------------------------------------------------
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