-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34268/#review84202
-----------------------------------------------------------


Thanks a lot for this, Stan - much appreciated!

There are a couple of style nits here and there and one basic question on the 
need of the `read`-variant for Solaris. 

For submitting an updated patch, please consult the patch submission guidelines 
http://mesos.apache.org/documentation/latest/submitting-a-patch/ specifically 
after "Submit your patch" - we need a patch that can be processed using our 
tooling and for that to work, an easy way is to follow that guide.


File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp
<https://reviews.apache.org//r/34268/#fcomment67>

    s/Linux/SunOS/



File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp
<https://reviews.apache.org//r/34268/#fcomment68>

    Not used?!



File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp
<https://reviews.apache.org//r/34268/#fcomment69>

    Not used?!



File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp
<https://reviews.apache.org//r/34268/#fcomment70>

    Please wrap to get below 80 chars per line.
    
    ```
      Try<Duration> utime =
        Seconds(pstatus.pr_utime.tv_sec) + 
Nanoseconds(pstatus.pr_utime.tv_nsec);
      Try<Duration> stime =
        Seconds(pstatus.pr_stime.tv_sec) + 
Nanoseconds(pstatus.pr_stime.tv_nsec);
    ```



File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp
<https://reviews.apache.org//r/34268/#fcomment71>

    Please wrap to stay below 80 chars per line.
    
    Also when looking at this patch with an editor, I noticed that your 
intendtion is partially off here - we use soft-tabs, 2 spaces for all C++ 
source files.
    
    ```
      return Process(pstatus.pr_pid,
                     pstatus.pr_ppid,
                     pstatus.pr_ppid,
                     pstatus.pr_sid,
                     None(),
                     utime.isSome() ? utime.get() : Option<Duration>::none(),
                     stime.isSome() ? stime.get() : Option<Duration>::none(),
                     psinfo.pr_fname,
                     (psinfo.pr_nzomb == 0) &&
                      (psinfo.pr_nlwp == 0) &&
                      (psinfo.pr_lwp.pr_lwpid == 0));
    
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135382>

    We commonly comment on the `#endif` from `#ifdef` `#endif` combinations 
quoting the clause.
    
    ```
    #endif // NAME_MAX
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135381>

    ```
    #endif // __sun
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135383>

    ```
    #endif // __sun
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135378>

    Complete sentence with punctuation, please.
    
    ```
    // FTS is not available on Solaris.
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135384>

    ```
    #endif // __sun
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/34268/#comment135371>

    Add a new line please.



3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp
<https://reviews.apache.org/r/34268/#comment135372>

    Use complete sentences with punctuation please:
    ```
    // Not defined on Solaris, taking a spare flag.
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/34268/#comment135377>

    See below on `read`.



3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/34268/#comment135373>

    Could you please explain why the standard implementation of this function 
would not work for Solaris?



3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/34268/#comment135376>

    Insert new line, please.



3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/34268/#comment135374>

    We do not commonly comment the `#endif` of a `#if define()` 
    ```
    #endif
    ```


Looking forward to give your updated patch another review, thanks again.

- Till Toenshoff


On May 15, 2015, 2:25 p.m., Stan Teresen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34268/
> -----------------------------------------------------------
> 
> (Updated May 15, 2015, 2:25 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> stout library - adding support for Solaris
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc 
> 
> Diff: https://reviews.apache.org/r/34268/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> adding missing new file: stout/os/sunos.hpp
>   
> https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp
> 
> 
> Thanks,
> 
> Stan Teresen
> 
>

Reply via email to