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 39803: Windows: Implemented stout/os/stat.hpp`.
Date Wed, 04 Nov 2015 21:42:19 GMT


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > This is my initial set of comments. I still need to read from line 112 on in stat.hpp
and I haven't started reading reparsepoint.hpp.

Replied to a few project-specific (we might call it "tribal knowledge") points you raised.

If you can, please drop some of them as appropriate (i.e. if no change is needed).


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 70
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line70>
> >
> >     Is FollowSymLink your enum or an enum that already exists in the Linux version?

This one already exists.  See: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp#L66-L70


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 91
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line91>
> >
> >     I'll just ask this once here - is string concatendation the accepted way of
building up error messages in Stout?

Yeah it is.  Almost everything else (besides messages for `Error`) uses streams though.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 107
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line107>
> >
> >     Is the enum+switch+unreachable a common pattern in stout? It seems that choosing
to use an enume for follw, instead of a bool made this code much more complex.
> >     
> >     Also, this is the only use of UNREACHABLE(), so you could remove the #include
if you reworked the function.
> >     
> >     Perhaps size() is an existing API in stout and you have no choice about its
prototype? In that case, are you sure that your version of enum FollowSymlink is exactly the
same as the one used in the Linux version. 
> >     
> >     You would hate for someone on the Linux side to take a dependency on the order
of the items inside the enum and find that you had changed that order.

Yes, `UNREACHABLE` is commonly used in enum-switch statements.  

There is another pattern of excluding a `default` and adding a blurb like:
```
// NOTE: By not setting a default we leverage the compiler
// errors when the enumeration is augmented to find all
// the cases we need to provide.
```
(We do end up using `UNREACHABLE` anyway in some of those cases too.)


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line
55
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line55>
> >
> >     Google Style Guide allows auto if it help readability? Is it a good or bad idea
here?

In general, we don't use `auto` unless it's completely obvious from the right-side what the
type is.  In this case, you might expect `os::realpath` to return a `std::string`, `Try<std::string>`,
`Option<str::string>`, `Path`, etc.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line
64
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line64>
> >
> >     Google Style Guide isn't really clear on this one. I would recommend indenting
line 66 to align with the open paren on line 65.

Generally, we would go for this:
```
return Error(
    "Reparse point attribute is not set for path '" +
    absolutePath.get() + "', and therefore it is not a sybolic link");
```
Note: no period at the end of the message.


> On Nov. 3, 2015, 5:02 p.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 83
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line83>
> >
> >     Google Style Guide discourages default arguments.

I think we're fine with default args.


- Joseph


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


On Oct. 31, 2015, 6:31 p.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2015, 6:31 p.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 67b142bbc2d80f40a1e893cc5813d58dd2aa8381

>   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/windows/stat.hpp 675b2e712358a55b3580026936890eaf80e5af71

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 1a7037d64afeedc340258c92067e95d1d3caa027

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