mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 62508: Fixed ordering of Windows system headers.
Date Mon, 02 Oct 2017 19:26:47 GMT

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


Ship it!




A couple nits, which I can fix before committing.


3rdparty/stout/include/stout/ip.hpp
Line 69 (original), 67 (patched)
<https://reviews.apache.org/r/62508/#comment263732>

    Nano-nit: Comment must start with a capital letter and end with a period.



3rdparty/stout/include/stout/windows.hpp
Lines 16-21 (original), 32-38 (patched)
<https://reviews.apache.org/r/62508/#comment263734>

    Going to remove this TODO as it does not have much to do with header ordering.  (Rather,
we can consider cleaning up this list of headers later, as some includes may no longer be
in use.)



3rdparty/stout/include/stout/windows/ip.hpp
Line 23 (original), 23-25 (patched)
<https://reviews.apache.org/r/62508/#comment263735>

    Hmm... a different style of comment?
    
    Since I prefer the one-line variety, I'll change it :)



3rdparty/stout/include/stout/windows/mac.hpp
Lines 23-24 (original), 23-26 (patched)
<https://reviews.apache.org/r/62508/#comment263736>

    Different style _and_ double inclusion? :)



3rdparty/stout/include/stout/windows/os.hpp
Lines 43-47 (patched)
<https://reviews.apache.org/r/62508/#comment263739>

    Going to plop down a comment here:
    ```
    // NOTE: These system headers must be included after `stout/windows.hpp`
    // as they may include `Windows.h`. See comments in `stout/windows.hpp`
    // for why this ordering is important.
    ```


- Joseph Wu


On Sept. 22, 2017, 11:21 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62508/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2017, 11:21 a.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7993
>     https://issues.apache.org/jira/browse/MESOS-7993
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch consolidates the inclusion of the ordering-dependent Windows
> system headers `WinSock2.h`, `WS2tcpip.h`, `Windows.h`, etc. For
> historical reasons, `Windows.h` will include `winsock.h`, the header for
> the Windows Sockets 1.1 library, which was last supported in Windows
> 2000. We use the Windows Sockets 2 library, which requires the
> `WinSock2.h` header, and this header must be included before
> `Windows.h`, otherwise symbol redefinition errors will occur. The
> `WS2tcpip.h` header includes the extensions to Windows Sockets 2, and
> any header which may include `Windows.h` must be included carefully.
> 
> It is simplest to consolidate the inclusion of these problematic system
> headers into `stout/windows.hpp`, and elsewhere include that with a
> comment as to which header specifically the file is requiring. Doing so
> will prevent incorrect ordering from being introduced.
> 
> Note that the erroneous inclusion of `winsock.h` was removed.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/ip.hpp a722fa47e05cf093e4ab4fed9d2824236dd5dd80 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 1c9dda0ba4653f970167e959139afb851682c6f8

>   3rdparty/stout/include/stout/os/windows/fd.hpp ae2db27154e694f319dafe2e04c01d55c42179de

>   3rdparty/stout/include/stout/os/windows/sendfile.hpp d50c89e4931b9109d7496fb4db496aea2ac7f830

>   3rdparty/stout/include/stout/os/windows/socket.hpp 18d2ecf560933d6a86cf945460b8611581b58cbb

>   3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed 
>   3rdparty/stout/include/stout/windows/ip.hpp d7738f2fe9cb6818e5686dcb7dbb3cc73618f856

>   3rdparty/stout/include/stout/windows/mac.hpp 3ebf4e15e81e882d52a769811757f713a8ae65df

>   3rdparty/stout/include/stout/windows/net.hpp 364509f62f365eb5c1fd3ffe9aa38d1cb677c131

>   3rdparty/stout/include/stout/windows/os.hpp a70a61c702a422462872c0ec85f3c34e26a2e383

> 
> 
> Diff: https://reviews.apache.org/r/62508/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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