mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Tue, 12 Jan 2016 02:07:34 GMT


> On Nov. 4, 2015, 6:08 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 130
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line130>
> >
> >     Might want to static_assert that _USE_32BIT_TIME_T is not defined. Probably
sound advice for _UNICODE and _MBCS as well. Perhaps these static asserts should go in windows.hpp?
If you put them there, add a comment saying why the asserts are necessary (e.g. "Implementation
of mtime() assumes ...")
> 
> Alex Clemmer wrote:
>     Thinking about it now, I think we should assert this at the top of `stat.hpp`, because
we're not guaranteed to include `windows.hpp` at all in general, and besides that, we're not
guaranteed to include it after defining `_USER_32BIT_TIME_T`.
>     
>     `stat.hpp` file is meant to wrap calls to `stat` anyway, so I think this is probably
sufficient.
>     
>     What do you think?
> 
> Alex Clemmer wrote:
>     As for `_MBCS` and `_UNICODE`, I think we can put such things in `windows.hpp`, but
I think there is probably a better place for them, considering that this really only will
be effective at protecting against -D flags passed into the compiler.

Alright, I've gone ahead and added checks for `_USER_32BIT_TIME_T` and `_UNICODE`, but `_MBCS`
seems to be enabled in VS projects by default. Consulting with Alex Naparu, we believe it
is not important to explicitly check that this flag is not set. We do agree that `_UNICODE`
should not be set because it makes POSIX compliance really hard.

Let us know if our understanding is not correct here.


- Alex


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


On Jan. 12, 2016, 2:03 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 2:03 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van Remoortere,
and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implemented stout/os/stat.hpp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe

>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp ffe074830ef90f492990bf695e686c885b9a155c

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 5b38b9af654d7d1c574f0cc573083b66693ced1d

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp d46e262e0fd1c2de36f3bf19d8bd693c23bf58cd

> 
> Diff: https://reviews.apache.org/r/39803/diff/
> 
> 
> Testing
> -------
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


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