----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4899/#review7702 -----------------------------------------------------------
src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16951> Related to the previous comment, I don't like something called create that also truncates. If the use case is to have a new truncated file, one can simply do: utils::os::rm(path); utils::os::touch(path); That is, I'd prefer this was the 'touch' abstraction we know and love, without the truncate. I would consider the ability to pass a 'bool truncate' flag to 'touch', but I'm not sure it's necessary. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16952> We really shouldn't be logging here (and everywhere else in utils, since they all return Try or Result). src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16953> Shouldn't need this line, just use 'strerror(errno)' on the next line. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16959> Okay, I reneg my previous comment. I think it makes sense to use Result for read because there are three possibilities: (1) the data; (2) an error; (3) EOF. We can represent EOF as Result::none. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16954> Did you want an 'else if' here? And you should kill this check anyway and return a Result::none below where you check if length == 0. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16961> Kill. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16962> s/open file./open " + path src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16960> Kill. src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16963> What if it's an error? If your intention is that it will never be an error, make this a CHECK first before you do a .get(). src/common/utils.hpp <https://reviews.apache.org/r/4899/#comment16964> Right, here, using Result makes sense (like the read above). Just all the other places didn't make sense. src/slave/slave.cpp <https://reviews.apache.org/r/4899/#comment16965> s/rea/Try< src/tests/utils_tests.cpp <https://reviews.apache.org/r/4899/#comment16966> UtilsTest should really be a fixture which makes sure the file gets deleted even if the test fails (see https://reviews.apache.org/r/4981 for an example). - Benjamin On 2012-05-04 00:28:13, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4899/ > ----------------------------------------------------------- > > (Updated 2012-05-04 00:28:13) > > > Review request for mesos and John Sirois. > > > Summary > ------- > > We can now read/write strings to a file. > > Some common utils for working with protobuf objects. > > > Diffs > ----- > > src/Makefile.am cd503a8 > src/common/protobuf_utils.hpp PRE-CREATION > src/common/utils.hpp 1d81e21 > src/slave/slave.cpp 09a8396 > src/tests/protobuf_io_tests.cpp 22f37ac > src/tests/utils_tests.cpp 49ec107 > > Diff: https://reviews.apache.org/r/4899/diff > > > Testing > ------- > > make check > > > Thanks, > > Vinod > >
