> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, > > line 61 > > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line61> > > > > Recommend opening a work item for Windows team to add this API. Who > > knows what version of Windows would actually ship it, but the world would > > be a slightly better place. > > > > Also wonder if you should add your code to a StackOverflow answer. > > Alex Clemmer wrote: > Who do you think we should contact about this?
Marking this as "dropped" and we'll take it offline. > On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line > > 64 > > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line64> > > > > If you always return symlink.isSome(), then why use Try<>? Is it that > > other users of querySymbolicLinkData() actually care about getting an error > > back? > > > > Even if you keep Try<> in the API, is it ok to silently ignore an error? > > Alex Clemmer wrote: > Yep, it's a `Try` because other places we call `querySymbolicLinkData` > care about the error. > > Whether it is ok to silently ignore an error depends on what you're > trying to do. In this case, if for any reason we are unable to retrieve > information about the symlink (via the `Symlink` struct), then we report that > it is not a symlink. So it is reasonable to ignore the error, because we > don't care what the error was. > > Right? Marking this as "dropped" because there's no action item. > On Nov. 4, 2015, 1:02 a.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. > > Joseph Wu wrote: > 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.) > > Benjamin Bannier wrote: > In light of https://issues.apache.org/jira/browse/MESOS-3754 I went and > checked how often we actually add a `default` branch when we `switch` over > enum values. This is the `clang-query` I used (which probably misses some > cases of fall-through into `default`): > > match switchStmt(has(declRefExpr(hasType(enumDecl()))), > > has(compoundStmt(has(defaultStmt().bind("default"))))) > > After checking all stout tests, their includes, and similarly all > libprocess `.cpp` files I could only find a single such case in > `os::stat::size`. > > We *do use* `default` branches when switching over int values though; > here the domain makes it impractical to enumerate all values ;) Marking this a "dropped" because it is used elsewhere and a fix seems outside the scope of this review. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39803/#review105007 ----------------------------------------------------------- On Jan. 5, 2016, 12:12 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39803/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2016, 12:12 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/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 > >
