-----------------------------------------------------------
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
> 
>

Reply via email to