Skip to content

Commit

Permalink
Fix MSVC build (#338)
Browse files Browse the repository at this point in the history
* Replace uses of `uint` with `uint32_t`
* Explicitly define `S2Point` constructor
  • Loading branch information
jherico authored Jan 10, 2024
1 parent fadf458 commit 1ff1671
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/s2/s1angle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ std::ostream& operator<<(std::ostream& os, S1Angle a) {
char buffer[13];
int sz = snprintf(buffer, sizeof(buffer), "%.7f", degrees);
// Fix sign/unsign comparison for client that use `-Wextra` (e.g. Chrome).
if (sz >= 0 && static_cast<uint>(sz) < sizeof(buffer)) {
if (sz >= 0 && static_cast<uint32_t>(sz) < sizeof(buffer)) {
return os << buffer;
} else {
return os << degrees;
Expand Down
20 changes: 10 additions & 10 deletions src/s2/s2lax_polygon_shape.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,19 +259,19 @@ inline S2Shape::ChainPosition S2LaxPolygonShape::chain_position(int e) const {
// Test if this edge belongs to the loop returned by the previous call.
const uint32* start = &loop_starts_[0] +
prev_loop_.load(std::memory_order_relaxed);
if (static_cast<uint>(e) >= start[0] && static_cast<uint>(e) < start[1]) {
if (static_cast<uint32_t>(e) >= start[0] && static_cast<uint32_t>(e) < start[1]) {
// This edge belongs to the same loop as the previous call.
} else {
if (static_cast<uint>(e) == start[1]) {
if (static_cast<uint32_t>(e) == start[1]) {
// This is the edge immediately following the previous loop.
do {
++start;
} while (static_cast<uint>(e) == start[1]);
} while (static_cast<uint32_t>(e) == start[1]);
} else {
start = &loop_starts_[0];
constexpr int kMaxLinearSearchLoops = 12; // From benchmarks.
if (num_loops() <= kMaxLinearSearchLoops) {
while (start[1] <= static_cast<uint>(e)) ++start;
while (start[1] <= static_cast<uint32_t>(e)) ++start;
} else {
start = std::upper_bound(start + 1, start + num_loops(), e) - 1;
}
Expand Down Expand Up @@ -304,20 +304,20 @@ inline S2Shape::ChainPosition EncodedS2LaxPolygonShape::chain_position(int e)
}
constexpr int kMaxLinearSearchLoops = 12; // From benchmarks.
int i = prev_loop_.load(std::memory_order_relaxed);
if (i == 0 && static_cast<uint>(e) < loop_starts_[1]) {
if (i == 0 && static_cast<uint32_t>(e) < loop_starts_[1]) {
return ChainPosition(0, e); // Optimization for first loop.
}
if (static_cast<uint>(e) >= loop_starts_[i] &&
static_cast<uint>(e) < loop_starts_[i + 1]) {
if (static_cast<uint32_t>(e) >= loop_starts_[i] &&
static_cast<uint32_t>(e) < loop_starts_[i + 1]) {
// This edge belongs to the same loop as the previous call.
} else {
if (static_cast<uint>(e) == loop_starts_[i + 1]) {
if (static_cast<uint32_t>(e) == loop_starts_[i + 1]) {
// This is the edge immediately following the previous loop.
do {
++i;
} while (static_cast<uint>(e) == loop_starts_[i + 1]);
} while (static_cast<uint32_t>(e) == loop_starts_[i + 1]);
} else if (num_loops() <= kMaxLinearSearchLoops) {
for (i = 0; loop_starts_[i + 1] <= static_cast<uint>(e); ++i) {
for (i = 0; loop_starts_[i + 1] <= static_cast<uint32_t>(e); ++i) {
}
} else {
i = loop_starts_.lower_bound(e + 1) - 1;
Expand Down
19 changes: 19 additions & 0 deletions src/s2/s2point.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,25 @@ class S2Point : public Vector3_d {
using Base = Vector3_d;
using Base::Base;

// Due to an ambiguity in original C++11 specificiation, it was unclear
// whether imported base class default constructors should be considered
// when deciding to delete the default constructor of a class. GCC and
// Clang both accept the base class default ctor, while MSVC 2017 and
// later do not.
// The ambiguity was resolved in
// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0136r1.html
// and accepted as part of the C++17 approval process, with the intent of
// being retroactively applied to C++11.
// However, while MSVC accepted the change for C++17 and forward, it did
// not implement the change for prior versions. See their conformance page:
// https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170
// (search for P0136R1).
// The explicit declaration here of a default ctor is a workaround until
// this codebase is targeted at C++17 and above, at which point MSVC, GCC
// and Clang will all have identical behavior and won't require this
// explicit declaration of a default ctor.
S2Point() = default;

// When S2Point was defined as a Vector3_d we could mix and match the two
// names. With inheritance upcasting to a Vector3_d is easy, but we need to
// explicitly allow the other direction, even though there's no data to
Expand Down
6 changes: 3 additions & 3 deletions src/s2/s2polygon.h
Original file line number Diff line number Diff line change
Expand Up @@ -962,11 +962,11 @@ inline S2Shape::ChainPosition S2Polygon::Shape::chain_position(int e) const {
}
} else {
i = prev_loop_.load(std::memory_order_relaxed);
if (static_cast<uint>(e) >= start[i] &&
static_cast<uint>(e) < start[i + 1]) {
if (static_cast<uint32_t>(e) >= start[i] &&
static_cast<uint32_t>(e) < start[i + 1]) {
// This edge belongs to the same loop as the previous call.
} else {
if (static_cast<uint>(e) == start[i + 1]) {
if (static_cast<uint32_t>(e) == start[i + 1]) {
// This edge immediately follows the loop from the previous call.
// Note that S2Polygon does not allow empty loops.
++i;
Expand Down

0 comments on commit 1ff1671

Please sign in to comment.