> On Nov. 15, 2012, 10:40 p.m., Ben Mahler wrote: > > src/linux/proc.hpp, line 57 > > <https://reviews.apache.org/r/8059/diff/1/?file=189981#file189981line57> > > > > does this not fit on one line? > > ditto for the initializer
Done. > On Nov. 15, 2012, 10:40 p.m., Ben Mahler wrote: > > third_party/libprocess/include/stout/stringify.hpp, line 72 > > <https://reviews.apache.org/r/8059/diff/1/?file=189984#file189984line72> > > > > 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! I choose that format to differentiate it from std::list (and because a vector is indexable via indices). But I agree, the indices in the output is redundant and unnecessary. Also fixed strings::join, thanks! Note that I didn't use strings::join here though because that would create circular dependencies. I'll leave that refactor to another day. > On Nov. 15, 2012, 10:40 p.m., Ben Mahler wrote: > > src/linux/proc.cpp, line 69 > > <https://reviews.apache.org/r/8059/diff/1/?file=189982#file189982line69> > > > > 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? I copied this code from the proc::stat() function (in this file). My guess is that originally I didn't do 'while (getline(...))' because getline doesn't actually return a bool and it requires relying on the semantics of casting an iostream object to a bool (or void* in this case). Likewise, I'm not sure if all the os.hpp facilities existed. Regardless, I've changed this to use the os::read(path) functions for now instead. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8059/#review13493 ----------------------------------------------------------- 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 > >
