> On March 8, 2017, 5:27 p.m., Michael Park wrote: > > 3rdparty/stout/include/stout/os/posix/stat.hpp > > Line 30 (original), 30 (patched) > > <https://reviews.apache.org/r/56027/diff/1/?file=1617902#file1617902line30> > > > > Do we want to return a `Try<::stat>` here, maybe? This way we can also > > factor out the > > ```cpp > > "Error invoking stat for '" + path + "'" > > ``` > > message here. > > James Peach wrote: > There's ~20 places that depend on this returning boolean. Returning `Try` > would complicate many of these call sites. > > Michael Park wrote: > Hm, sorry, I'm not following... could you elaborate? > > From my perspective, I count 10 instances where this is used (all in this > patch since it was introduced here). > > I only see 2 patterns that would change, and they both seem pretty simple > to me. > > ```cpp > struct stat s; > > if (internal::stat(path, follow, s) < 0) { > return false; > } > > return S_ISXXX(s.st_mode); > ``` > to > ```cpp > Try<::stat> stat = internal::stat(path, follow); > if (stat.isError()) { > return false; > } > > return S_ISXXX(stat.get().st_mode); > ``` > > and... > > ```cpp > struct stat s; > > if (internal::stat(path, follow, s) < 0) { > return ErrnoError("Error invoking stat for '" + path + "'"); > } > > // do stuff with `s`... > ``` > to > ```cpp > Try<::stat> stat = internal::stat(path, follow); > if (stat.isError()) { > return Error("Failed to perform XXX" + stat.error()); > } > > // do stuff with `stat.get()`... > ``` > > What are the complications that I'm missing here?
I thought you meant changing `stat::isdir` to return a `Try`? I'll apply your suggestion without changing the signature of `isdir()`. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56027/#review168311 ----------------------------------------------------------- On March 8, 2017, 11:55 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56027/ > ----------------------------------------------------------- > > (Updated March 8, 2017, 11:55 p.m.) > > > Review request for mesos, Alex Clemmer, Joris Van Remoortere, Joseph Wu, > Michael Park, and Jiang Yan Xu. > > > Bugs: MESOS-7021 > https://issues.apache.org/jira/browse/MESOS-7021 > > > Repository: mesos > > > Description > ------- > > Consistently add the FollowSymlink stat argument on POSIX. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/posix/stat.hpp > e7440a4dab82ca3758a60a8fdc859476b2ee5693 > 3rdparty/stout/include/stout/os/stat.hpp > 5c4fd4e630459059fa94c9f98e0d9b73d1280918 > > > Diff: https://reviews.apache.org/r/56027/diff/3/ > > > Testing > ------- > > make check (Fedora 25) > > > Thanks, > > James Peach > >
