> On Sept. 20, 2016, 10:16 a.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>`."); > > ``` > > Benjamin Bannier wrote: > 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.
ah, right. awesome. thanks for the analysis! - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52025/#review149634 ----------------------------------------------------------- On Sept. 19, 2016, 10:19 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52025/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2016, 10:19 a.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 > >
