mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joris Van Remoortere <joris.van.remoort...@gmail.com>
Subject Re: Review Request 41632: Windows: Forked signal handling in `signalhandler.hpp`.
Date Wed, 02 Mar 2016 00:40:45 GMT

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



It's not clear from the JIRA (https://issues.apache.org/jira/browse/MESOS-3644) what we are
using this for.


3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 94)
<https://reviews.apache.org/r/41632/#comment183274>

    style
    alphabetize



3rdparty/libprocess/3rdparty/stout/include/Makefile.am (lines 116 - 117)
<https://reviews.apache.org/r/41632/#comment183275>

    style
    alphabetize



3rdparty/libprocess/3rdparty/stout/include/Makefile.am (lines 134 - 135)
<https://reviews.apache.org/r/41632/#comment183276>

    style
    alphabetize



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 21)
<https://reviews.apache.org/r/41632/#comment183278>

    separate block.



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 22)
<https://reviews.apache.org/r/41632/#comment183277>

    this comes first in a separate block



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 26)
<https://reviews.apache.org/r/41632/#comment183281>

    can we avoid the typedef?
    It's not that big to write out explicitly (We're already doing it in a few places)



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 26 - 28)
<https://reviews.apache.org/r/41632/#comment183283>

    How does this library enforce that this is only used in a single module?
    
    This seems extremely dangerous?



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 27)
<https://reviews.apache.org/r/41632/#comment183282>

    periods at the end of comments.



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 29 - 36)
<https://reviews.apache.org/r/41632/#comment183279>

    2 new lines between functions in a namespace



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 47)
<https://reviews.apache.org/r/41632/#comment183285>

    backticks around `sigaction()`



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 54)
<https://reviews.apache.org/r/41632/#comment183280>

    indentation



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 16 -
23)
<https://reviews.apache.org/r/41632/#comment183287>

    double includes?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 24)
<https://reviews.apache.org/r/41632/#comment183289>

    separate block



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 25)
<https://reviews.apache.org/r/41632/#comment183288>

    this goes by itself at the top



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 28)
<https://reviews.apache.org/r/41632/#comment183290>

    periods at the end of comments.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 28 -
30)
<https://reviews.apache.org/r/41632/#comment183292>

    Same question as above. How does the library enforce this single use in a single module?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 29)
<https://reviews.apache.org/r/41632/#comment183291>

    Same typedef concern as above.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 34 -
48)
<https://reviews.apache.org/r/41632/#comment183293>

    style.
    Please see other switch statements.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 47)
<https://reviews.apache.org/r/41632/#comment183294>

    indentation?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 54)
<https://reviews.apache.org/r/41632/#comment183295>

    style: whitespace
    between end of expression and brace. `){` => `) {`



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 56 -
57)
<https://reviews.apache.org/r/41632/#comment183296>

    `} else {`


- Joris Van Remoortere


On March 1, 2016, 11:56 p.m., Daniel Pravat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41632/
> -----------------------------------------------------------
> 
> (Updated March 1, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere,
Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Forked signal handling in `signalhandler.hpp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/41632/diff/
> 
> 
> Testing
> -------
> 
> OSX: make check
> Windows: make
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message