----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37442/#review97320 -----------------------------------------------------------
Some notes before you rebase. src/linux/perf.cpp (line 490) <https://reviews.apache.org/r/37442/#comment153163> Why not make this an Option<uint64_t> and when it's <not counted> this the value is None? src/linux/perf.cpp (line 498) <https://reviews.apache.org/r/37442/#comment153164> How about 'parse' to be consistent with how we've named these kinds of functions, perhaps making it a static member of Sample? No need for inline here? src/linux/perf.cpp (lines 500 - 512) <https://reviews.apache.org/r/37442/#comment153165> Indentation is off here, please have a look over the diff, also s/if(/if (/ :) src/linux/perf.cpp (lines 501 - 504) <https://reviews.apache.org/r/37442/#comment153166> Don't forget to update this when you rebase. src/linux/perf.cpp (line 511) <https://reviews.apache.org/r/37442/#comment153168> The caller is expected to print the line when composing the error message, the callee here should print why the line is bad. src/linux/perf.cpp (lines 522 - 524) <https://reviews.apache.org/r/37442/#comment153169> Let's include the line contents in the error message by composing a message with the parse error: ``` return Error("Failed to parse perf sample line '" + line + "': " + parse.error()); ``` src/linux/perf.cpp (lines 531 - 577) <https://reviews.apache.org/r/37442/#comment153170> Can you now use the -> operator here? Might also avoid the need to wrap these, although, the "<not counted>" logic should be moved to 'parse()'. src/linux/perf.cpp (lines 540 - 543) <https://reviews.apache.org/r/37442/#comment153171> Now that you have a parse function, let's put the <not supported> logic there as well. - Ben Mahler On Aug. 31, 2015, 10:33 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37442/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2015, 10:33 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > ------- > > Factor out the token extraction rules in prepartion for extending them to > cope with multiple versions. > > > Diffs > ----- > > src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 > > Diff: https://reviews.apache.org/r/37442/diff/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Paul Brett > >
