----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18549/#review35651 -----------------------------------------------------------
Ship it! With these abstractions now it seems like the guide will be (at least in libprocess and Mesos): (1) Use ABORT where you need signal safety and want to abort. No conditional for ABORT is provided and should be done explicitly. (2) Use LOG(FATAL) and CHECK* where you want to abort otherwise and don't need signal safety (and do or don't want a conditional). We should kill fatal in subsequent reviews then. 3rdparty/libprocess/3rdparty/stout/include/stout/abort_msg.hpp <https://reviews.apache.org/r/18549/#comment66283> s/signal/Signal/ 3rdparty/libprocess/3rdparty/stout/include/stout/abort_msg.hpp <https://reviews.apache.org/r/18549/#comment66285> Why ABORT_MSG instead of just ABORT? 3rdparty/libprocess/3rdparty/stout/include/stout/abort_msg.hpp <https://reviews.apache.org/r/18549/#comment66282> '{' on newline please. 3rdparty/libprocess/3rdparty/stout/include/stout/abort_msg.hpp <https://reviews.apache.org/r/18549/#comment66284> Please kill white spaces. 3rdparty/libprocess/3rdparty/stout/include/stout/check.hpp <https://reviews.apache.org/r/18549/#comment66302> How about s/CheckFailed/CheckFailure/ or s/CheckFailed/CheckFatal/ to differentiate the CHECK_FAILED case in libprocess? I'm happy to see a better name than 'Failure' too (since that's overloaded with libprocess futures) but I'd like to avoid 'Failed' (originally looking at these reviews I thought '_CheckFailed' was just for the CHECK_FAILED case). 3rdparty/libprocess/3rdparty/stout/include/stout/check.hpp <https://reviews.apache.org/r/18549/#comment66295> There has always been a bug here: we need to append the error here rather than down in the constructor otherwise any appended strings via '<<' will end up being written in between (and be confusing and hard to read). Just like 'expression', this will let you kill 'error' as an instance member too! 3rdparty/libprocess/3rdparty/stout/include/stout/check.hpp <https://reviews.apache.org/r/18549/#comment66286> I'd prefer to keep the google::LogMessageFatal here to mimic the semantics of CHECK. In particular, the LogMessageFatal will include a stack trace. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/18549/#comment66288> Why not use UNREACHABLE here? - Benjamin Hindman On Feb. 27, 2014, 12:30 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18549/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2014, 12:30 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1041 > https://issues.apache.org/jira/browse/MESOS-1041 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/Makefile.am > 5d5a76021900bf27113fe5b730cfd2b6a537d132 > 3rdparty/libprocess/3rdparty/stout/include/stout/abort_msg.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/check.hpp > 695db20cb1acff036d8d493be4ff4b5e002fdd90 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp > 3350929686e5365225f72f48d7c99b42c57fd1ad > > Diff: https://reviews.apache.org/r/18549/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
