Skip to content

Commit

Permalink
[pzstd] Fixes for Windows build
Browse files Browse the repository at this point in the history
* Add `Portability.h` to fix min/max issues.
* Fix conversion warnings
* Assert that windowLog <= 23, which is currently always the case.
  This could be loosened, but we aren't looking to add new functionality.

Fixes on top of PR facebook#3375 by @eli-schwartz, which added Windows CI for contrib & programs.
  • Loading branch information
terrelln committed Dec 19, 2022
1 parent 9ab1213 commit ed2cff0
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 3 deletions.
9 changes: 8 additions & 1 deletion contrib/pzstd/Pzstd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
#include "Pzstd.h"
#include "SkippableFrame.h"
#include "utils/FileSystem.h"
#include "utils/Portability.h"
#include "utils/Range.h"
#include "utils/ScopeGuard.h"
#include "utils/ThreadPool.h"
#include "utils/WorkQueue.h"

#include <algorithm>
#include <chrono>
#include <cinttypes>
#include <cstddef>
Expand Down Expand Up @@ -336,6 +338,10 @@ static size_t calculateStep(
const ZSTD_parameters &params) {
(void)size;
(void)numThreads;
// Not validated to work correctly for window logs > 23.
// It will definitely fail if windowLog + 2 is >= 4GB because
// the skippable frame can only store sizes up to 4GB.
assert(params.cParams.windowLog <= 23);
return size_t{1} << (params.cParams.windowLog + 2);
}

Expand Down Expand Up @@ -587,7 +593,8 @@ std::uint64_t writeFile(
// start writing before compression is done because we need to know the
// compressed size.
// Wait for the compressed size to be available and write skippable frame
SkippableFrame frame(out->size());
assert(uint64_t(out->size()) < uint64_t(1) << 32);
SkippableFrame frame(uint32_t(out->size()));
if (!writeData(frame.data(), outputFd)) {
errorHolder.setError("Failed to write output");
return bytesWritten;
Expand Down
6 changes: 4 additions & 2 deletions contrib/pzstd/utils/FileSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
*/
#pragma once

#include "utils/Portability.h"
#include "utils/Range.h"

#include <sys/stat.h>
#include <cerrno>
#include <cstdint>
#include <limits>
#include <system_error>

// A small subset of `std::filesystem`.
Expand Down Expand Up @@ -82,11 +84,11 @@ inline std::uintmax_t file_size(
std::error_code& ec) noexcept {
auto stat = status(path, ec);
if (ec) {
return -1;
return std::numeric_limits<uintmax_t>::max();
}
if (!is_regular_file(stat)) {
ec.assign(ENOTSUP, std::generic_category());
return -1;
return std::numeric_limits<uintmax_t>::max();
}
ec.clear();
return stat.st_size;
Expand Down
16 changes: 16 additions & 0 deletions contrib/pzstd/utils/Portability.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright (c) 2016-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under both the BSD-style license (found in the
* LICENSE file in the root directory of this source tree) and the GPLv2 (found
* in the COPYING file in the root directory of this source tree).
*/

#pragma once

#include <algorithm>

// Required for windows, which defines min/max, but we want the std:: version.
#undef min
#undef max
2 changes: 2 additions & 0 deletions contrib/pzstd/utils/Range.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
#pragma once

#include "utils/Likely.h"
#include "utils/Portability.h"

#include <algorithm>
#include <cstddef>
#include <cstring>
#include <stdexcept>
Expand Down

0 comments on commit ed2cff0

Please sign in to comment.