> On Sept. 20, 2016, 12:16 p.m., Michael Park wrote: > > Just curious whether this is just a drive-by cleanup, or whether it's > > caused us problems? > > > > Also, could we add a `static_assert`? something like: > > ``` > > static_assert( > > std::is_base_of<Process, T>::value, > > "`Process<T>` requires the CRTP pattern. Please have `T` inherit from > > `Process<T>`."); > > ```
This is a drive-by fix triggered by this coverity report https://scan5.coverity.com/reports.htm#v42942/p10429/fileInstanceId=99295949&defectInstanceId=28546354&mergedDefectId=1063931. Coverity warns that `dynamic_cast` could in principle return a nullptr here. AFAIK this has not caused any issues (this code has been around since 2013). Re:`static_assert`, do we need this? We perform a `static_cast` of `this` to `T*` (this is of course all at compile time) so compilers can and already do verify that inheritance relationship for us, e.g., with template <typename T> struct Base { virtual ~Base() = default; const T *get() const { return static_cast<const T *>(this); } }; struct A { virtual ~A() = default; }; struct B : public Base<A> { virtual ~B() = default; }; int main() { B b; b.get(); } I get with clang-3.9 test.cpp:3:12: error: static_cast from 'const Base<A> *' to 'const A *', which are not related by inheritance, is not allowed return static_cast<const T *>(this); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ test.cpp:13:5: note: in instantiation of member function 'Base<A>::get' requested here b.get(); ^ 1 error generated. and with g++-6 test.cpp: In instantiation of 'const T* Base<T>::get() const [with T = A]': test.cpp:13:9: required from here test.cpp:3:12: error: invalid static_cast from type 'const Base<A>*' to type 'const A*' return static_cast<const T *>(this); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ I am not sure if any of this is required, but also cannot imagine anything useful a compiler could do with code where `T` and `Process` are not part of the same inheritance hierarchy (and that is what `std::is_base_of` checks). Note that `dynamic_cast` can perform cross casts, and don't seem to trigger these warnings here. I did add a `static_assert` to `self` and a compile of the code base under OS X brought up no issues. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52025/#review149634 ----------------------------------------------------------- On Sept. 19, 2016, 12:19 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52025/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2016, 12:19 p.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > ------- > > `Process` is a CRTP class where we know that `T` is a valid derived > class. Also, it is customary to use `static_cast` when implementing > CRTP methods. > > This change uses a `static_cast` instead of a `dynamic_cast`; > `dynamic_cast` can result in a `nullptr` which should be checked. Here > this is not required, so `static_cast` is good enough for this use > case. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/process.hpp > 54c7d2e7ed3923ab15ab86e36552b023f9de5215 > > Diff: https://reviews.apache.org/r/52025/diff/ > > > Testing > ------- > > make check (OS X, clang/trunk w/o optimizations) > > > Thanks, > > Benjamin Bannier > >
