You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/09/22 18:21:27 UTC
Review Request 62508: Fixed ordering of Windows system headers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62508/
-----------------------------------------------------------
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
Re: Review Request 62508: Fixed ordering of Windows system headers.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Oct. 2, 2017, 12:26 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/ip.hpp
> > Line 69 (original), 67 (patched)
> > <https://reviews.apache.org/r/62508/diff/1/?file=1832904#file1832904line69>
> >
> > Nano-nit: Comment must start with a capital letter and end with a period.
Oh. Yeah. That's right.
> On Oct. 2, 2017, 12:26 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/ip.hpp
> > Line 23 (original), 23-25 (patched)
> > <https://reviews.apache.org/r/62508/diff/1/?file=1832910#file1832910line23>
> >
> > Hmm... a different style of comment?
> >
> > Since I prefer the one-line variety, I'll change it :)
I staged these one-by-one to make sure I'd fully converted them to one-line `// for ...`... how I missed it, I don't know.
> On Oct. 2, 2017, 12:26 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp
> > Lines 43-47 (patched)
> > <https://reviews.apache.org/r/62508/diff/1/?file=1832913#file1832913line49>
> >
> > 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.
> > ```
Perfect.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62508/#review186870
-----------------------------------------------------------
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
>
>
Re: Review Request 62508: Fixed ordering of Windows system headers.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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
>
>