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




3rdparty/stout/CMakeLists.txt
Line 35 (original), 36 (patched)
<https://reviews.apache.org/r/67065/#comment285501>

    We shouldn't need to do this if we set the `INTERFACE_LINK_LIBRARIES` 
property of the `libarchive` target correctly.



3rdparty/stout/include/stout/archiver.hpp
Lines 27-32 (patched)
<https://reviews.apache.org/r/67065/#comment285502>

    If you run `clang-format` on the file, I think it'll sort these out 
correctly.



3rdparty/stout/include/stout/archiver.hpp
Lines 36-40 (patched)
<https://reviews.apache.org/r/67065/#comment285506>

    I think these were meant to be above `extract()`.



3rdparty/stout/include/stout/archiver.hpp
Lines 42-43 (patched)
<https://reviews.apache.org/r/67065/#comment285503>

    Style: in stout we use `snake_case` instead of `camelCase`.



3rdparty/stout/include/stout/archiver.hpp
Lines 45-56 (patched)
<https://reviews.apache.org/r/67065/#comment285505>

    I can't make sense of this function. What is the meaning of its return 
value and out parameters?



3rdparty/stout/include/stout/archiver.hpp
Lines 59-62 (patched)
<https://reviews.apache.org/r/67065/#comment285525>

    This needs to be documented. Is `destination` the folder in to which the 
archive will be extracted?



3rdparty/stout/include/stout/archiver.hpp
Lines 68 (patched)
<https://reviews.apache.org/r/67065/#comment285510>

    If you wanted, you could replace `std::function<void(struct archive*)>` 
with `declspec(&archive_read_close)`.



3rdparty/stout/include/stout/archiver.hpp
Lines 68-70 (patched)
<https://reviews.apache.org/r/67065/#comment285512>

    Wait, this is C++, can't we `s/struct archive/archive`? (Thinking aloud 
here; should we typedef it locally so the typename is Capitalized?)



3rdparty/stout/include/stout/archiver.hpp
Lines 84 (patched)
<https://reviews.apache.org/r/67065/#comment285513>

    s/insure/ensure



3rdparty/stout/include/stout/archiver.hpp
Lines 86 (patched)
<https://reviews.apache.org/r/67065/#comment285514>

    Third argument can be ommitted since `0` is the default.



3rdparty/stout/include/stout/archiver.hpp
Lines 89 (patched)
<https://reviews.apache.org/r/67065/#comment285515>

    Delete most of that and just `return Error(fd.error())`.



3rdparty/stout/include/stout/archiver.hpp
Lines 93 (patched)
<https://reviews.apache.org/r/67065/#comment285516>

    Does libarchive really require a CRT file descriptor, it's not happy with a 
`HANDLE`?



3rdparty/stout/include/stout/archiver.hpp
Lines 103 (patched)
<https://reviews.apache.org/r/67065/#comment285519>

    Well we just leaked a handle (or worse) because a CRT fd was allocated 
above, and not closed. I think this is undefined behavior on Windows (closing a 
`HANDLE` with `CloseHandle` when a CRT fd has been allocated; the docs just say 
don't do it).



3rdparty/stout/include/stout/archiver.hpp
Lines 115-118 (patched)
<https://reviews.apache.org/r/67065/#comment285521>

    I think the interface we're looking for is more like:
    
    ```
    // Returns `Nothing()` if ARCHIVE_OK, otherwise returns an `Error`.
    Try<Nothing> get_archive_error(int, archive);
    ...
    Try<Nothing> error = get_archive_error(result, writer.get());
    if (error.isError()) {
      // Don't have to close anything because the FDs should be wrapped in a 
deleter.
      return error;
    }
    ```



3rdparty/stout/include/stout/archiver.hpp
Lines 126 (patched)
<https://reviews.apache.org/r/67065/#comment285522>

    Are we _sure_ this handles long paths internally? We're not adding `\\?\` 
ourselves...



3rdparty/stout/include/stout/archiver.hpp
Lines 136 (patched)
<https://reviews.apache.org/r/67065/#comment285523>

    Nit: Mesos style attaches `*` to the type, not the name. (Run clang-format!)



3rdparty/stout/include/stout/archiver.hpp
Lines 165 (patched)
<https://reviews.apache.org/r/67065/#comment285526>

    What's the ordering between `ARCHIVE_OK` and `ARCHIVE_WARN`? I don't 
understand these `<` comparisons without knowning the enumeration of `result`.



3rdparty/stout/tests/CMakeLists.txt
Line 103 (original), 104 (patched)
<https://reviews.apache.org/r/67065/#comment285527>

    This isn't necessary as the `target_link_libraries` command adds this 
dependency.



3rdparty/stout/tests/CMakeLists.txt
Lines 106 (patched)
<https://reviews.apache.org/r/67065/#comment285528>

    I think actually you meant to add `libarchive` to the  stout `INTERFACE` 
library, which means all users of it (like `stout-tests` or `libmesos` etc.) 
would be pick it up transitively.


- Andrew Schwartzmeyer


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> -----------------------------------------------------------
> 
> (Updated May 14, 2018, 3:42 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
>     https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/3/
> 
> 
> Testing
> -------
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Reply via email to