mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mp...@apache.org>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Thu, 14 Jan 2016 17:56:30 GMT


> On Jan. 14, 2016, midnight, Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 126
> > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line126>
> >
> >     I noticed the use of `WindowsError` below. Should this return `WindowsError`
as well? Here and below.
> 
> Alex Clemmer wrote:
>     Probably not. `WindowsError` is similar to `ErrnoError`, but instead of capturing
the error information from `errno` (which is populated when the C standard library encounters
a runtime error), it captures the error from `GetLastError` (which is populated when the Windows
API encounters a runtime error). At this point in this function, we will not have had the
opportunity to trigger a runtime error in the Windows API. Hence, we would not use `WindowsError`.

I see, cool. Thanks!


> On Jan. 14, 2016, midnight, Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
lines 158-162
> > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line158>
> >
> >     ```
> >     bool resolvedPathIsDirectory =
> >       ::_stat(absolutePath.c_str(), &s) >= 0 && S_ISDIR(s.st_mode);
> >     ```
> 
> Alex Clemmer wrote:
>     I actually would like to keep this how it is, if you don't mind. I think it is less
clear the other way, and it's not that much shorter.

Ok, it's not a big deal. I actually meant to suggest:

```cpp
const bool resolvedPathIsDirectory =
  ::_stat(absolutePath.c_str(), &s) >= 0 && S_ISDIR(s.st_mode);
```

Just to clarify the motivation for the suggestion, it's not for brevity.
It's more so that we can't tell whether we're done initializing or not, looking at the following
piece of code:

```cpp
bool resolvedPathIsDirectory = false;

if (::_stat(absolutePath.c_str(), &s) >= 0) {
  resolvedPathIsDirectory = S_ISDIR(s.st_mode);
}

// Hm, are there more `if` statements below where we set `resolvedPathIsDirectory` differently?
```

We could also achieve this by using a lambda, which is the recommended approach in the C++
community although not prevalent in Mesos yet.

```cpp
const bool resolvedPathIsDirectory = [] {
  bool result = false;
  if (::_stat(absolutePath.c_str(), &s) >= 0) {
    result = S_ISDIR(s.st_mode);
  }
  return result;
}();
```


> On Jan. 14, 2016, midnight, Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp,
line 229
> > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line229>
> >
> >     It seems like we pass the address of this variable as a dummy out parameter
below. Presumably this can't be declared `const`. It looks like would be that C-style cast
of `(LPDWORD)&ignored` ignores the `const` and makes it so. This induces undefined behavior.
> 
> Alex Clemmer wrote:
>     Oh I had no idea. Thanks for telling me.
>     
>     I'm obviously no expert, but just for my own understanding: this _may_ induce undefined
behavior, if `DeviceIoControl` actually modifies `ignored`, right? But casting from const
is not in itself undefined behavior?
>     
>     Either way, this is good to know.

Yep, you're correct. Casting away `const` itself is legal, but modifying a `const` variable
via a pointer or reference that had its `const` casted away induces undefined behavior. Presumably
`DeviceIoControl` does write to it, since it's documented as an "out" parameter.


- Michael


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


On Jan. 14, 2016, 10:08 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2016, 10:08 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 ec851dcb08d938defeb6066810011e003d594b29

>   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 27edf1b4f8647a2720f2962eafa110d694b3baed

> 
> 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