----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8059/#review13493 -----------------------------------------------------------
src/linux/proc.hpp <https://reviews.apache.org/r/8059/#comment28939> does this not fit on one line? ditto for the initializer src/linux/proc.cpp <https://reviews.apache.org/r/8059/#comment28942> I think you're examining a stale line here, without checking for file.eof() after getline(). Alternatively, your loop could be as follows: string line; while (getline(file, line)) { ... } if (file.fail()) { return Try<...>::error("Failed to read /proc/cpuinfo"); } I think this will read much cleaner since we don't need to check for eof at all. Then again, why not use os.hpp facilities for this? third_party/libprocess/include/stout/stringify.hpp <https://reviews.apache.org/r/8059/#comment28951> why did you choose this format? {0: "a", 1: "b"} looks like a map, no? ["a", "b"] makes more sense. Also, please just do: out << '[' << strings::join(", ", vector) << ']'; And please fix strings::join()! Looks like you've got a bug there that sticks the separator before the first element! - Ben Mahler On Nov. 14, 2012, 11:59 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8059/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2012, 11:59 p.m.) > > > Review request for mesos, Vinod Kone and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/linux/proc.hpp 27e15bf8695aa694b0d5bdb6881b9fa55a447528 > src/linux/proc.cpp bf8b7ec6000af9d485188359bf1c36d533632af2 > src/tests/proc_tests.cpp 3d8318c74491b01502bd34583996ffeeee9826e7 > third_party/libprocess/include/stout/stringify.hpp > dcc5b95a54e6f34f93867e015d8c855fd7d6f950 > > Diff: https://reviews.apache.org/r/8059/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
