> On Oct. 24, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.cpp, line 204 > > <https://reviews.apache.org/r/7620/diff/2/?file=179327#file179327line204> > > > > I'd prefer this visually if you killed the newlines between the > > > > Try cpu = ...; > > > > if (cpu.isError()) { > > ... > > } > > > > to become: > > > > Try cpu = ...; > > if (cpu.isError()) { > > ... > > } > > > > In all the error checks in this function, it makes each logical chunk > > more obvious IMO.
Okay, I changed this one, but a little reluctantly. In most circumstances I completely agree, but there is a lot of text here and by merging these together makes the important calls no longer "pop" (e.g., readControl, readControl, writeControl, writeControl). In general I'd prefer we use the more compact style unless it makes finding the "meat" of the function more difficult. > On Oct. 24, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.cpp, line 330 > > <https://reviews.apache.org/r/7620/diff/2/?file=179327#file179327line330> > > > > Is jie's TODO here still applicable? > > > > I think he's referring to getting errno out of std::ostringstream. I'm going to leave that for now. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7620/#review12726 ----------------------------------------------------------- On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7620/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2012, 4:40 a.m.) > > > Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21 > src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb > third_party/libprocess/include/stout/path.hpp > e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b > > Diff: https://reviews.apache.org/r/7620/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
