From b0c380aa2ba14a98e434eb232166c890a70d31e7 Mon Sep 17 00:00:00 2001 From: Jesse Rosenstock Date: Mon, 6 Dec 2021 12:43:49 +0100 Subject: [PATCH 1/2] Update to latest google3 version * EncodedS2CellIdVector: Detect invalid cell levels and return failure * Use Decoder::skip(0) instead of deprecated ptr() * Deprecate more functions * Make S2LaxPolylineShape moveable * Improve comments --- doc/examples/point_index.cc | 2 +- doc/examples/term_index.cc | 5 +-- src/s2/encoded_s2cell_id_vector.cc | 2 + src/s2/encoded_s2cell_id_vector_test.cc | 28 ++++++++++++ src/s2/encoded_s2point_vector.cc | 4 +- src/s2/encoded_string_vector.cc | 2 +- src/s2/encoded_uint_vector.h | 4 +- src/s2/r1interval.h | 5 ++- src/s2/s2cell_id.h | 10 +++-- src/s2/s2centroids.cc | 4 +- src/s2/s2closest_cell_query_base.h | 1 + src/s2/s2edge_crossings.h | 8 ++-- src/s2/s2edge_distances.h | 5 ++- src/s2/s2edge_vector_shape_test.cc | 9 ++-- src/s2/s2lax_polyline_shape.cc | 14 ++++++ src/s2/s2lax_polyline_shape.h | 8 ++++ src/s2/s2lax_polyline_shape_test.cc | 19 ++++++++ src/s2/s2loop.cc | 4 +- src/s2/s2loop_measures.cc | 6 +-- src/s2/s2metrics.h | 32 +++++++------- src/s2/s2metrics_test.cc | 17 ++++++-- src/s2/s2point_vector_shape_test.cc | 5 +-- src/s2/s2pointutil.h | 1 + src/s2/s2polygon.h | 3 ++ src/s2/s2region_term_indexer_test.cc | 18 ++++++++ src/s2/s2shape_index_region.h | 1 + src/s2/s2text_format.cc | 26 ----------- src/s2/s2text_format.h | 58 ++++++++++++++++--------- src/s2/util/bits/bits.h | 5 +++ 29 files changed, 207 insertions(+), 99 deletions(-) diff --git a/doc/examples/point_index.cc b/doc/examples/point_index.cc index 0f51726d..9262c410 100644 --- a/doc/examples/point_index.cc +++ b/doc/examples/point_index.cc @@ -42,5 +42,5 @@ int main(int argc, char **argv) { std::printf("Found %" PRId64 " points in %d queries\n", num_found, absl::GetFlag(FLAGS_num_queries)); - return 0; + return 0; } diff --git a/doc/examples/term_index.cc b/doc/examples/term_index.cc index eeb87cb9..b7d38816 100644 --- a/doc/examples/term_index.cc +++ b/doc/examples/term_index.cc @@ -14,7 +14,6 @@ #include #include #include -#include #include #include "s2/base/commandlineflags.h" @@ -35,7 +34,7 @@ DEFINE_double(query_radius_km, 100, "Query radius in kilometers"); // (e.g. representing words or phrases). static const char kPrefix[] = "s2:"; -int main(int argc, char **argv) { +int main(int argc, char** argv) { // Create a set of "documents" to be indexed. Each document consists of a // single point. (You can easily substitute any S2Region type here, or even // index a mixture of region types using std::unique_ptr. Other @@ -98,5 +97,5 @@ int main(int argc, char **argv) { } std::printf("Found %" PRId64 " points in %d queries\n", num_found, absl::GetFlag(FLAGS_num_queries)); - return 0; + return 0; } diff --git a/src/s2/encoded_s2cell_id_vector.cc b/src/s2/encoded_s2cell_id_vector.cc index 2be7aa91..2429a705 100644 --- a/src/s2/encoded_s2cell_id_vector.cc +++ b/src/s2/encoded_s2cell_id_vector.cc @@ -139,7 +139,9 @@ bool EncodedS2CellIdVector::Init(Decoder* decoder) { int shift_code = code_plus_len >> 3; if (shift_code == 31) { shift_code = 29 + decoder->get8(); + if (shift_code > 56) return false; // Valid range 0..56 } + // Decode the "base_len" most-significant bytes of "base". int base_len = code_plus_len & 7; if (!DecodeUintWithLength(base_len, decoder, &base_)) return false; diff --git a/src/s2/encoded_s2cell_id_vector_test.cc b/src/s2/encoded_s2cell_id_vector_test.cc index 7ef6c5f3..39de8b85 100644 --- a/src/s2/encoded_s2cell_id_vector_test.cc +++ b/src/s2/encoded_s2cell_id_vector_test.cc @@ -18,8 +18,10 @@ #include "s2/encoded_s2cell_id_vector.h" #include + #include #include "absl/memory/memory.h" +#include "s2/s2cell_id.h" #include "s2/s2loop.h" #include "s2/s2pointutil.h" #include "s2/s2shape_index.h" @@ -31,6 +33,7 @@ using s2textformat::MakeCellIdOrDie; using std::vector; namespace s2coding { +namespace { // Encodes the given vector and returns the corresponding // EncodedS2CellIdVector (which points into the Encoder's data buffer). @@ -139,6 +142,30 @@ TEST(EncodedS2CellIdVector, OneByteRangeWithBaseValue) { 0x0100500000000000, 0x0100330000000000}, 9); } +TEST(EncodedS2CellIdVector, MaxShiftRange) { + const std::vector bytes = { + (31 << 3) // 31 -> add 29 to bytes[1]. + + 1, // Number of encoded cell IDs. + 27, // 27+29 is the maximum supported shift. + 1, 0 // Encoded cell ID. Not important. + }; + Decoder decoder(bytes.data(), bytes.size()); + EncodedS2CellIdVector cell_ids; + EXPECT_TRUE(cell_ids.Init(&decoder)); +} + +TEST(EncodedS2CellIdVector, ShiftOutOfRange) { + const std::vector bytes = { + (31 << 3) // 31 -> add 29 to bytes[1]. + + 1, // Number of encoded cell IDs. + 28, // 28+29 is greater than the maximum supported shift of 56. + 1, 0 // Encoded cell ID. Not important. + }; + Decoder decoder(bytes.data(), bytes.size()); + EncodedS2CellIdVector cell_ids; + EXPECT_FALSE(cell_ids.Init(&decoder)); +} + TEST(EncodedS2CellIdVector, SixFaceCells) { vector ids; for (int face = 0; face < 6; ++face) { @@ -229,4 +256,5 @@ TEST(EncodedS2CellIdVector, LowerBoundLimits) { EXPECT_EQ(2, cell_ids.lower_bound(S2CellId::Sentinel())); } +} // namespace } // namespace s2coding diff --git a/src/s2/encoded_s2point_vector.cc b/src/s2/encoded_s2point_vector.cc index 8c6d476b..82d53280 100644 --- a/src/s2/encoded_s2point_vector.cc +++ b/src/s2/encoded_s2point_vector.cc @@ -105,7 +105,7 @@ bool EncodedS2PointVector::Init(Decoder* decoder) { // Peek at the format but don't advance the decoder; the format-specific // Init functions will do that. - format_ = static_cast(*decoder->ptr() & kEncodingFormatMask); + format_ = static_cast(*decoder->skip(0) & kEncodingFormatMask); switch (format_) { case UNCOMPRESSED: return InitUncompressedFormat(decoder); @@ -196,7 +196,7 @@ bool EncodedS2PointVector::InitUncompressedFormat(Decoder* decoder) { size_t bytes = size_t{size_} * sizeof(S2Point); if (decoder->avail() < bytes) return false; - uncompressed_.points = reinterpret_cast(decoder->ptr()); + uncompressed_.points = reinterpret_cast(decoder->skip(0)); decoder->skip(bytes); return true; } diff --git a/src/s2/encoded_string_vector.cc b/src/s2/encoded_string_vector.cc index 08f28577..1be42f37 100644 --- a/src/s2/encoded_string_vector.cc +++ b/src/s2/encoded_string_vector.cc @@ -46,7 +46,7 @@ void StringVectorEncoder::Encode(Span v, Encoder* encoder) { bool EncodedStringVector::Init(Decoder* decoder) { if (!offsets_.Init(decoder)) return false; - data_ = reinterpret_cast(decoder->ptr()); + data_ = decoder->skip(0); if (offsets_.size() > 0) { uint64 length = offsets_[offsets_.size() - 1]; if (decoder->avail() < length) return false; diff --git a/src/s2/encoded_uint_vector.h b/src/s2/encoded_uint_vector.h index c54b9b4e..439ced25 100644 --- a/src/s2/encoded_uint_vector.h +++ b/src/s2/encoded_uint_vector.h @@ -191,7 +191,7 @@ inline T GetUintWithLength(const char* ptr, int length) { template bool DecodeUintWithLength(int length, Decoder* decoder, T* result) { if (decoder->avail() < length) return false; - const char* ptr = reinterpret_cast(decoder->ptr()); + const char* ptr = decoder->skip(0); *result = GetUintWithLength(ptr, length); decoder->skip(length); return true; @@ -230,7 +230,7 @@ bool EncodedUintVector::Init(Decoder* decoder) { if (size_ > std::numeric_limits::max() / sizeof(T)) return false; size_t bytes = size_ * len_; if (decoder->avail() < bytes) return false; - data_ = reinterpret_cast(decoder->ptr()); + data_ = decoder->skip(0); decoder->skip(bytes); return true; } diff --git a/src/s2/r1interval.h b/src/s2/r1interval.h index f1208038..ba0b8c26 100644 --- a/src/s2/r1interval.h +++ b/src/s2/r1interval.h @@ -66,8 +66,9 @@ class R1Interval { } } - // Accessors methods. + // The low bound of the interval. double lo() const { return bounds_[0]; } + // The high bound of the interval. double hi() const { return bounds_[1]; } // Methods to modify one endpoint of an existing R1Interval. Do not use @@ -97,10 +98,12 @@ class R1Interval { // is negative. double GetLength() const { return hi() - lo(); } + // Returns true if the given point is in the closed interval [lo, hi]. bool Contains(double p) const { return p >= lo() && p <= hi(); } + // Returns true if the given point is in the open interval (lo, hi). bool InteriorContains(double p) const { return p > lo() && p < hi(); } diff --git a/src/s2/s2cell_id.h b/src/s2/s2cell_id.h index 2d1221f2..cc3d1497 100644 --- a/src/s2/s2cell_id.h +++ b/src/s2/s2cell_id.h @@ -78,9 +78,10 @@ class S2LatLng; // the default copy constructor and assignment operator. class S2CellId { public: - // The extra position bit (61 rather than 60) let us encode each cell as its - // Hilbert curve position at the cell center (which is halfway along the - // portion of the Hilbert curve that fills that cell). + // Although only 60 bits are needed to represent the index of a leaf cell, the + // extra position bit lets us encode each cell as its Hilbert curve position + // at the cell center, which is halfway along the portion of the Hilbert curve + // that fills that cell. static constexpr int kFaceBits = 3; static constexpr int kNumFaces = 6; static constexpr int kMaxLevel = @@ -94,7 +95,7 @@ class S2CellId { explicit IFNDEF_SWIG(constexpr) S2CellId(uint64 id) : id_(id) {} // Construct a leaf cell containing the given point "p". Usually there is - // is exactly one such cell, but for points along the edge of a cell, any + // exactly one such cell, but for points along the edge of a cell, any // adjacent cell may be (deterministically) chosen. This is because // S2CellIds are considered to be closed sets. The returned cell will // always contain the given point, i.e. @@ -113,6 +114,7 @@ class S2CellId { // The default constructor returns an invalid cell id. IFNDEF_SWIG(constexpr) S2CellId() : id_(0) {} + // Returns an invalid cell id. static constexpr S2CellId None() { return S2CellId(); } // Returns an invalid cell id guaranteed to be larger than any diff --git a/src/s2/s2centroids.cc b/src/s2/s2centroids.cc index e6ec2314..f5241bb3 100644 --- a/src/s2/s2centroids.cc +++ b/src/s2/s2centroids.cc @@ -57,8 +57,8 @@ S2Point TrueCentroid(const S2Point& a, const S2Point& b, const S2Point& c) { // The result is the true centroid of the triangle multiplied by the // triangle's area. // - // TODO(ericv): This code still isn't as numerically stable as it could be. - // The biggest potential improvement is to compute B-A and C-A more + // TODO(b/205027737): This code still isn't as numerically stable as it could + // be. The biggest potential improvement is to compute B-A and C-A more // accurately so that (B-A)x(C-A) is always inside triangle ABC. S2Point x(a.x(), b.x() - a.x(), c.x() - a.x()); S2Point y(a.y(), b.y() - a.y(), c.y() - a.y()); diff --git a/src/s2/s2closest_cell_query_base.h b/src/s2/s2closest_cell_query_base.h index fccace5b..01993a3f 100644 --- a/src/s2/s2closest_cell_query_base.h +++ b/src/s2/s2closest_cell_query_base.h @@ -25,6 +25,7 @@ #include "s2/base/logging.h" #include "absl/container/btree_set.h" #include "absl/container/inlined_vector.h" +#include "absl/hash/hash.h" #include "s2/s1chord_angle.h" #include "s2/s2cap.h" #include "s2/s2cell_id.h" diff --git a/src/s2/s2edge_crossings.h b/src/s2/s2edge_crossings.h index e6b8f70e..6952fcf9 100644 --- a/src/s2/s2edge_crossings.h +++ b/src/s2/s2edge_crossings.h @@ -138,10 +138,10 @@ bool AngleContainsVertex(const S2Point& a, const S2Point& b, const S2Point& c); // Given two edges AB and CD where at least two vertices are identical // (i.e. CrossingSign(a,b,c,d) == 0), this function defines whether the -// two edges "cross" in a such a way that point-in-polygon containment tests -// can be implemented by counting the number of edge crossings. The basic -// rule is that a "crossing" occurs if AB is encountered after CD during a -// CCW sweep around the shared vertex starting from a fixed reference point. +// two edges "cross" in such a way that point-in-polygon containment tests can +// be implemented by counting the number of edge crossings. The basic rule is +// that a "crossing" occurs if AB is encountered after CD during a CCW sweep +// around the shared vertex starting from a fixed reference point. // // Note that according to this rule, if AB crosses CD then in general CD // does not cross AB. However, this leads to the correct result when diff --git a/src/s2/s2edge_distances.h b/src/s2/s2edge_distances.h index faa04040..677ea88e 100644 --- a/src/s2/s2edge_distances.h +++ b/src/s2/s2edge_distances.h @@ -25,6 +25,7 @@ #include #include "s2/base/logging.h" +#include "absl/base/macros.h" #include "s2/s1angle.h" #include "s2/s1chord_angle.h" #include "s2/s2edge_crossings.h" @@ -206,12 +207,12 @@ S2Point GetPointOnRay(const S2Point& origin, const S2Point& dir, constexpr S1Angle kGetPointOnRayPerpendicularError = S1Angle::Radians( 3 * s2pred::DBL_ERR); -ABSL_DEPRECATED("Use Interpolate(a, b, t) instead.") +ABSL_DEPRECATED("Inline the implementation") inline S2Point Interpolate(double t, const S2Point& a, const S2Point& b) { return Interpolate(a, b, t); } -ABSL_DEPRECATED("Use GetPointOnLine(a, b, ax) instead.") +ABSL_DEPRECATED("Inline the implementation") inline S2Point InterpolateAtDistance(S1Angle ax, const S2Point& a, const S2Point& b) { return GetPointOnLine(a, b, ax); diff --git a/src/s2/s2edge_vector_shape_test.cc b/src/s2/s2edge_vector_shape_test.cc index 1b239f01..b495aa42 100644 --- a/src/s2/s2edge_vector_shape_test.cc +++ b/src/s2/s2edge_vector_shape_test.cc @@ -35,22 +35,23 @@ TEST(S2EdgeVectorShape, EdgeAccess) { S2EdgeVectorShape shape; S2Testing::rnd.Reset(absl::GetFlag(FLAGS_s2_random_seed)); const int kNumEdges = 100; + std::vector> edges; for (int i = 0; i < kNumEdges; ++i) { S2Point a = S2Testing::RandomPoint(); // Control the evaluation order - shape.Add(a, S2Testing::RandomPoint()); + edges.emplace_back(a, S2Testing::RandomPoint()); + shape.Add(edges.back().first, edges.back().second); } EXPECT_EQ(kNumEdges, shape.num_edges()); EXPECT_EQ(kNumEdges, shape.num_chains()); EXPECT_EQ(1, shape.dimension()); EXPECT_FALSE(shape.is_empty()); EXPECT_FALSE(shape.is_full()); - S2Testing::rnd.Reset(absl::GetFlag(FLAGS_s2_random_seed)); for (int i = 0; i < kNumEdges; ++i) { EXPECT_EQ(i, shape.chain(i).start); EXPECT_EQ(1, shape.chain(i).length); auto edge = shape.edge(i); - EXPECT_EQ(S2Testing::RandomPoint(), edge.v0); - EXPECT_EQ(S2Testing::RandomPoint(), edge.v1); + EXPECT_EQ(edges.at(i).first, edge.v0); + EXPECT_EQ(edges.at(i).second, edge.v1); } } diff --git a/src/s2/s2lax_polyline_shape.cc b/src/s2/s2lax_polyline_shape.cc index a2b30a21..83d91f74 100644 --- a/src/s2/s2lax_polyline_shape.cc +++ b/src/s2/s2lax_polyline_shape.cc @@ -18,13 +18,27 @@ #include "s2/s2lax_polyline_shape.h" #include +#include +#include + #include "s2/base/logging.h" +#include "absl/utility/utility.h" #include "s2/s2polyline.h" using absl::make_unique; using absl::MakeSpan; using absl::Span; +S2LaxPolylineShape::S2LaxPolylineShape(S2LaxPolylineShape&& other) : + num_vertices_(absl::exchange(other.num_vertices_, 0)), + vertices_(std::move(other.vertices_)) {} + +S2LaxPolylineShape& S2LaxPolylineShape::operator=(S2LaxPolylineShape&& other) { + num_vertices_ = absl::exchange(other.num_vertices_, 0); + vertices_ = std::move(other.vertices_); + return *this; +} + S2LaxPolylineShape::S2LaxPolylineShape(Span vertices) { Init(vertices); } diff --git a/src/s2/s2lax_polyline_shape.h b/src/s2/s2lax_polyline_shape.h index eb5f2f98..e97159fa 100644 --- a/src/s2/s2lax_polyline_shape.h +++ b/src/s2/s2lax_polyline_shape.h @@ -18,8 +18,10 @@ #ifndef S2_S2LAX_POLYLINE_SHAPE_H_ #define S2_S2LAX_POLYLINE_SHAPE_H_ +#include #include #include + #include "absl/types/span.h" #include "s2/encoded_s2point_vector.h" #include "s2/s2polyline.h" @@ -40,6 +42,12 @@ class S2LaxPolylineShape : public S2Shape { // Constructs an empty polyline. S2LaxPolylineShape() : num_vertices_(0) {} + // Move constructor. + S2LaxPolylineShape(S2LaxPolylineShape&&); + + // Move-assignment operator. + S2LaxPolylineShape& operator=(S2LaxPolylineShape&&); + // Constructs an S2LaxPolylineShape with the given vertices. explicit S2LaxPolylineShape(absl::Span vertices); diff --git a/src/s2/s2lax_polyline_shape_test.cc b/src/s2/s2lax_polyline_shape_test.cc index 84f61736..fd45cddf 100644 --- a/src/s2/s2lax_polyline_shape_test.cc +++ b/src/s2/s2lax_polyline_shape_test.cc @@ -17,6 +17,8 @@ #include "s2/s2lax_polyline_shape.h" +#include + #include #include "s2/s2shapeutil_testing.h" #include "s2/s2text_format.h" @@ -44,6 +46,23 @@ TEST(S2LaxPolylineShape, OneVertex) { EXPECT_FALSE(shape.is_full()); } +TEST(S2LaxPolylineShape, MoveConstructor) { + std::unique_ptr original = + s2textformat::MakeLaxPolylineOrDie("1:1, 4:4"); + S2LaxPolylineShape moved(std::move(*original)); + ASSERT_EQ(0, original->num_vertices()); + ASSERT_EQ(2, moved.num_vertices()); +} + +TEST(S2LaxPolylineShape, MoveAssignmentOperator) { + std::unique_ptr original = + s2textformat::MakeLaxPolylineOrDie("1:1, 4:4"); + S2LaxPolylineShape moved; + moved = std::move(*original); + ASSERT_EQ(0, original->num_vertices()); + ASSERT_EQ(2, moved.num_vertices()); +} + TEST(S2LaxPolylineShape, EdgeAccess) { vector vertices = s2textformat::ParsePoints("0:0, 0:1, 1:1"); S2LaxPolylineShape shape(vertices); diff --git a/src/s2/s2loop.cc b/src/s2/s2loop.cc index 0f44f677..10a3ba22 100644 --- a/src/s2/s2loop.cc +++ b/src/s2/s2loop.cc @@ -637,11 +637,11 @@ bool S2Loop::DecodeInternal(Decoder* const decoder, bool is_misaligned = false; #else bool is_misaligned = - reinterpret_cast(decoder->ptr()) % sizeof(double) != 0; + reinterpret_cast(decoder->skip(0)) % sizeof(double) != 0; #endif if (within_scope && !is_misaligned) { vertices_ = const_cast(reinterpret_cast( - decoder->ptr())); + decoder->skip(0))); decoder->skip(num_vertices_ * sizeof(*vertices_)); owns_vertices_ = false; } else { diff --git a/src/s2/s2loop_measures.cc b/src/s2/s2loop_measures.cc index 68c6f9b6..f38e7b69 100644 --- a/src/s2/s2loop_measures.cc +++ b/src/s2/s2loop_measures.cc @@ -52,7 +52,7 @@ double GetArea(S2PointLoopSpan loop) { } double GetSignedArea(S2PointLoopSpan loop) { - // It is suprisingly difficult to compute the area of a loop robustly. The + // It is surprisingly difficult to compute the area of a loop robustly. The // main issues are (1) whether degenerate loops are considered to be CCW or // not (i.e., whether their area is close to 0 or 4*Pi), and (2) computing // the areas of small loops with good relative accuracy. @@ -228,8 +228,8 @@ double GetCurvatureMaxError(S2PointLoopSpan loop) { // ------------------- // 11.25 * DBL_EPSILON // - // TODO(ericv): This error estimate is approximate. There are two issues: - // (1) SignedArea needs some improvements to ensure that its error is + // TODO(b/203697029): This error estimate is approximate. There are two + // issues: (1) SignedArea needs some improvements to ensure that its error is // actually never higher than GirardArea, and (2) although the number of // triangles in the sum is typically N-2, in theory it could be as high as // 2*N for pathological inputs. But in other respects this error bound is diff --git a/src/s2/s2metrics.h b/src/s2/s2metrics.h index a9441f8e..b2365157 100644 --- a/src/s2/s2metrics.h +++ b/src/s2/s2metrics.h @@ -50,24 +50,24 @@ template class Metric { // particular metric. double GetValue(int level) const { return ldexp(deriv_, - dim * level); } - // Return the level at which the metric has approximately the given - // value. For example, S2::kAvgEdge.GetClosestLevel(0.1) returns the - // level at which the average cell edge length is approximately 0.1. - // The return value is always a valid level. + // Return the level at which the metric has approximately the given value. + // For example, S2::kAvgEdge.GetClosestLevel(0.1) returns the level at which + // the average cell edge length is approximately 0.1. The return value is + // always a valid level. int GetClosestLevel(double value) const; - // Return the minimum level such that the metric is at most the given - // value, or S2CellId::kMaxLevel if there is no such level. For example, + // Return the minimum level such that the metric is at most the given value, + // or S2CellId::kMaxLevel if there is no such level. For example, // S2::kMaxDiag.GetLevelForMaxValue(0.1) returns the minimum level such // that all cell diagonal lengths are 0.1 or smaller. The return value // is always a valid level. int GetLevelForMaxValue(double value) const; - // Return the maximum level such that the metric is at least the given - // value, or zero if there is no such level. For example, - // S2::kMinWidth.GetLevelForMinValue(0.1) returns the maximum level such - // that all cells have a minimum width of 0.1 or larger. The return value - // is always a valid level. + // Return the maximum level such that the metric is at least the given value, + // or 0 if there is no such level. For example, + // S2::kMinWidth.GetLevelForMinValue(0.1) returns the maximum level such that + // all cells have a minimum width of 0.1 or larger. The return value is + // always a valid level. int GetLevelForMinValue(double value) const; private: @@ -166,9 +166,9 @@ template int S2::Metric::GetLevelForMaxValue(double value) const { if (value <= 0) return S2::kMaxCellLevel; - // This code is equivalent to computing a floating-point "level" - // value and rounding up. ilogb() returns the exponent corresponding to a - // fraction in the range [1,2). + // This code is equivalent to computing a floating-point "level" value and + // rounding up. ilogb() returns the exponent corresponding to a fraction in + // the range [1,2). int level = ilogb(value / deriv_); level = std::max(0, std::min(S2::kMaxCellLevel, -(level >> (dim - 1)))); S2_DCHECK(level == S2::kMaxCellLevel || GetValue(level) <= value); @@ -180,8 +180,8 @@ template int S2::Metric::GetLevelForMinValue(double value) const { if (value <= 0) return S2::kMaxCellLevel; - // This code is equivalent to computing a floating-point "level" - // value and rounding down. + // This code is equivalent to computing a floating-point "level" value and + // rounding down. int level = ilogb(deriv_ / value); level = std::max(0, std::min(S2::kMaxCellLevel, level >> (dim - 1))); S2_DCHECK(level == 0 || GetValue(level) >= value); diff --git a/src/s2/s2metrics_test.cc b/src/s2/s2metrics_test.cc index ccd153a4..05e93511 100644 --- a/src/s2/s2metrics_test.cc +++ b/src/s2/s2metrics_test.cc @@ -89,13 +89,24 @@ TEST(S2, Metrics) { EXPECT_LE(S2::kMaxArea.deriv(), S2::kMaxWidth.deriv() * S2::kMaxEdge.deriv() + 1e-15); + // The minimum level for which the minimum or maximum width of a cell is at + // most 0 is kMaxCellLevel, because no cell at any level has width less than + // or equal to zero. + EXPECT_EQ(S2::kMinWidth.GetLevelForMaxValue(0), S2::kMaxCellLevel); + EXPECT_EQ(S2::kMaxWidth.GetLevelForMaxValue(0), S2::kMaxCellLevel); + + // The maximum level for which the minimum or maximum width of a cell is at + // least 4 is 0, because no cell at any level has width greater than 4. + EXPECT_EQ(S2::kMinWidth.GetLevelForMinValue(4), 0); + EXPECT_EQ(S2::kMaxWidth.GetLevelForMinValue(4), 0); + // GetLevelForMaxValue() and friends have built-in assertions, we just need // to call these functions to test them. // // We don't actually check that the metrics are correct here, e.g. that - // GetMinWidth(10) is a lower bound on the width of cells at level 10. - // It is easier to check these properties in s2cell_test, since - // S2Cell has methods to compute the cell vertices, etc. + // GetLevelForMaxValue(1) is a lower bound on the level of cells with width 1. + // It is easier to check these properties in s2cell_test, since S2Cell has + // methods to compute the cell vertices, etc. for (int level = -2; level <= S2::kMaxCellLevel + 3; ++level) { double width = S2::kMinWidth.deriv() * pow(2, -level); diff --git a/src/s2/s2point_vector_shape_test.cc b/src/s2/s2point_vector_shape_test.cc index d35a4096..eca61d2d 100644 --- a/src/s2/s2point_vector_shape_test.cc +++ b/src/s2/s2point_vector_shape_test.cc @@ -41,19 +41,18 @@ TEST(S2PointVectorShape, ConstructionAndAccess) { for (int i = 0; i < kNumPoints; ++i) { points.push_back(S2Testing::RandomPoint()); } - S2PointVectorShape shape(std::move(points)); + S2PointVectorShape shape(points); EXPECT_EQ(kNumPoints, shape.num_edges()); EXPECT_EQ(kNumPoints, shape.num_chains()); EXPECT_EQ(0, shape.dimension()); EXPECT_FALSE(shape.is_empty()); EXPECT_FALSE(shape.is_full()); - S2Testing::rnd.Reset(absl::GetFlag(FLAGS_s2_random_seed)); for (int i = 0; i < kNumPoints; ++i) { EXPECT_EQ(i, shape.chain(i).start); EXPECT_EQ(1, shape.chain(i).length); auto edge = shape.edge(i); - S2Point pt = S2Testing::RandomPoint(); + const S2Point& pt = points.at(i); EXPECT_EQ(pt, edge.v0); EXPECT_EQ(pt, edge.v1); } diff --git a/src/s2/s2pointutil.h b/src/s2/s2pointutil.h index ae333280..c5884578 100644 --- a/src/s2/s2pointutil.h +++ b/src/s2/s2pointutil.h @@ -35,6 +35,7 @@ namespace S2 { // Returns a unique "origin" on the sphere for operations that need a fixed // reference point. In particular, this is the "point at infinity" used for // point-in-polygon testing (by counting the number of edge crossings). +// To be clear, this is NOT (0,0,0), the origin of the coordinate system. inline S2Point Origin(); // Returns true if the given point is approximately unit length diff --git a/src/s2/s2polygon.h b/src/s2/s2polygon.h index f89891c7..fa98bf03 100644 --- a/src/s2/s2polygon.h +++ b/src/s2/s2polygon.h @@ -18,9 +18,12 @@ #ifndef S2_S2POLYGON_H_ #define S2_S2POLYGON_H_ +#include #include #include #include +#include +#include #include #include "s2/base/integral_types.h" diff --git a/src/s2/s2region_term_indexer_test.cc b/src/s2/s2region_term_indexer_test.cc index 0091c064..7ac42c52 100644 --- a/src/s2/s2region_term_indexer_test.cc +++ b/src/s2/s2region_term_indexer_test.cc @@ -32,6 +32,7 @@ #include "s2/s2cell.h" #include "s2/s2cell_id.h" #include "s2/s2cell_union.h" +#include "s2/s2latlng.h" #include "s2/s2testing.h" using std::string; @@ -167,6 +168,23 @@ TEST(S2RegionTermIndexer, IndexPointsQueryRegionsOptimizeSpace) { TestRandomCaps(options, QueryType::CAP); } +TEST(S2RegionTermIndexer, MarkerCharacter) { + S2RegionTermIndexer::Options options; + options.set_min_level(20); + options.set_max_level(20); + + S2RegionTermIndexer indexer(options); + S2Point point = S2LatLng::FromDegrees(10, 20).ToPoint(); + EXPECT_EQ(indexer.options().marker_character(), '$'); + EXPECT_EQ(indexer.GetQueryTerms(point, ""), + vector({"11282087039", "$11282087039"})); + + indexer.mutable_options()->set_marker_character(':'); + EXPECT_EQ(indexer.options().marker_character(), ':'); + EXPECT_EQ(indexer.GetQueryTerms(point, ""), + vector({"11282087039", ":11282087039"})); +} + TEST(S2RegionTermIndexer, MaxLevelSetLoosely) { // Test that correct terms are generated even when (max_level - min_level) // is not a multiple of level_mod. diff --git a/src/s2/s2shape_index_region.h b/src/s2/s2shape_index_region.h index 2510d40b..7b90e183 100644 --- a/src/s2/s2shape_index_region.h +++ b/src/s2/s2shape_index_region.h @@ -388,6 +388,7 @@ bool S2ShapeIndexRegion::VisitIntersectingShapes( return true; } } + S2_LOG(FATAL) << "Unhandled S2ShapeIndex::CellRelation"; } template diff --git a/src/s2/s2text_format.cc b/src/s2/s2text_format.cc index 685d89c1..64a12ba7 100644 --- a/src/s2/s2text_format.cc +++ b/src/s2/s2text_format.cc @@ -201,11 +201,6 @@ bool MakePolyline(string_view str, unique_ptr* polyline, return true; } -std::unique_ptr MakePolyline(string_view str, - S2Debug debug_override) { - return MakePolylineOrDie(str, debug_override); -} - unique_ptr MakeLaxPolylineOrDie(string_view str) { unique_ptr lax_polyline; S2_CHECK(MakeLaxPolyline(str, &lax_polyline)) << ": str == \"" << str << "\""; @@ -220,10 +215,6 @@ bool MakeLaxPolyline(string_view str, return true; } -std::unique_ptr MakeLaxPolyline(string_view str) { - return MakeLaxPolylineOrDie(str); -} - static bool InternalMakePolygon(string_view str, S2Debug debug_override, bool normalize_loops, @@ -255,11 +246,6 @@ bool MakePolygon(string_view str, unique_ptr* polygon, return InternalMakePolygon(str, debug_override, true, polygon); } -std::unique_ptr MakePolygon(string_view str, - S2Debug debug_override) { - return MakePolygonOrDie(str, debug_override); -} - unique_ptr MakeVerbatimPolygonOrDie(string_view str) { unique_ptr polygon; S2_CHECK(MakeVerbatimPolygon(str, &polygon)) << ": str == \"" << str << "\""; @@ -270,10 +256,6 @@ bool MakeVerbatimPolygon(string_view str, unique_ptr* polygon) { return InternalMakePolygon(str, S2Debug::ALLOW, false, polygon); } -std::unique_ptr MakeVerbatimPolygon(string_view str) { - return MakeVerbatimPolygonOrDie(str); -} - unique_ptr MakeLaxPolygonOrDie(string_view str) { unique_ptr lax_polygon; S2_CHECK(MakeLaxPolygon(str, &lax_polygon)) << ": str == \"" << str << "\""; @@ -297,10 +279,6 @@ bool MakeLaxPolygon(string_view str, return true; } -std::unique_ptr MakeLaxPolygon(string_view str) { - return MakeLaxPolygonOrDie(str); -} - unique_ptr MakeIndexOrDie(string_view str) { auto index = make_unique(); S2_CHECK(MakeIndex(str, &index)) << ": str == \"" << str << "\""; @@ -333,10 +311,6 @@ bool MakeIndex(string_view str, std::unique_ptr* index) { return true; } -std::unique_ptr MakeIndex(string_view str) { - return MakeIndexOrDie(str); -} - static void AppendVertex(const S2LatLng& ll, string* out) { absl::StrAppendFormat(out, "%.15g:%.15g", ll.lat().degrees(), ll.lng().degrees()); diff --git a/src/s2/s2text_format.h b/src/s2/s2text_format.h index a8cb9c19..d2ccdbcb 100644 --- a/src/s2/s2text_format.h +++ b/src/s2/s2text_format.h @@ -28,13 +28,18 @@ #include #include "absl/base/attributes.h" -#include "absl/types/span.h" +#include "absl/base/macros.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "s2/s2cell_id.h" #include "s2/s2cell_union.h" #include "s2/s2debug.h" #include "s2/s2latlng_rect.h" +#include "s2/s2lax_polygon_shape.h" // TODO(user,b/207351837): Remove. +#include "s2/s2lax_polyline_shape.h" // TODO(user,b/207351837): Remove. #include "s2/s2point_span.h" +#include "s2/s2polygon.h" // TODO(user,b/207351837): Remove. +#include "s2/s2polyline.h" // TODO(user,b/207351837): Remove. class MutableS2ShapeIndex; class S2LaxPolygonShape; @@ -55,7 +60,7 @@ S2Point MakePointOrDie(absl::string_view str); // is successful. ABSL_MUST_USE_RESULT bool MakePoint(absl::string_view str, S2Point* point); -ABSL_DEPRECATED("Use MakePointOrDie.") +ABSL_DEPRECATED("Inline the implementation") inline S2Point MakePoint(absl::string_view str) { return MakePointOrDie(str); } // Parses a string of one or more latitude-longitude coordinates in degrees, @@ -71,7 +76,7 @@ std::vector ParseLatLngsOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool ParseLatLngs(absl::string_view str, std::vector* latlngs); -ABSL_DEPRECATED("Use ParseLatLngsOrDie.") +ABSL_DEPRECATED("Inline the implementation") inline std::vector ParseLatLngs(absl::string_view str) { return ParseLatLngsOrDie(str); } @@ -85,7 +90,7 @@ std::vector ParsePointsOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool ParsePoints(absl::string_view str, std::vector* vertices); -ABSL_DEPRECATED("Use ParsePointsOrDie.") +ABSL_DEPRECATED("Inline the implementation") inline std::vector ParsePoints(absl::string_view str) { return ParsePointsOrDie(str); } @@ -106,7 +111,7 @@ S2LatLngRect MakeLatLngRectOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool MakeLatLngRect(absl::string_view str, S2LatLngRect* rect); -ABSL_DEPRECATED("Use MakeLatLngRectOrDie.") +ABSL_DEPRECATED("Inline the implementation") inline S2LatLngRect MakeLatLngRect(absl::string_view str) { return MakeLatLngRectOrDie(str); } @@ -161,10 +166,11 @@ ABSL_MUST_USE_RESULT bool MakePolyline(absl::string_view str, std::unique_ptr* polyline, S2Debug debug_override = S2Debug::ALLOW); -ABSL_DEPRECATED("Use MakePolylineOrDie.") -std::unique_ptr MakePolyline( - absl::string_view str, - S2Debug debug_override = S2Debug::ALLOW); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakePolyline( + absl::string_view str, S2Debug debug_override = S2Debug::ALLOW) { + return MakePolylineOrDie(str, debug_override); +} // Like MakePolyline, but returns an S2LaxPolylineShape instead. std::unique_ptr MakeLaxPolylineOrDie(absl::string_view str); @@ -174,8 +180,11 @@ std::unique_ptr MakeLaxPolylineOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool MakeLaxPolyline( absl::string_view str, std::unique_ptr* lax_polyline); -ABSL_DEPRECATED("Use MakeLaxPolylineOrDie.") -std::unique_ptr MakeLaxPolyline(absl::string_view str); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakeLaxPolyline( + absl::string_view str) { + return MakeLaxPolylineOrDie(str); +} // Given a sequence of loops separated by semicolons, returns a newly // allocated polygon. Loops are automatically normalized by inverting them @@ -200,9 +209,11 @@ ABSL_MUST_USE_RESULT bool MakePolygon(absl::string_view str, std::unique_ptr* polygon, S2Debug debug_override = S2Debug::ALLOW); -ABSL_DEPRECATED("Use MakePolygonOrDie.") -std::unique_ptr MakePolygon(absl::string_view str, - S2Debug debug_override = S2Debug::ALLOW); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakePolygon( + absl::string_view str, S2Debug debug_override = S2Debug::ALLOW) { + return MakePolygonOrDie(str, debug_override); +} // Like MakePolygon(), except that it does not normalize loops (i.e., it // gives you exactly what you asked for). @@ -213,8 +224,10 @@ std::unique_ptr MakeVerbatimPolygonOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool MakeVerbatimPolygon( absl::string_view str, std::unique_ptr* polygon); -ABSL_DEPRECATED("Use MakeVerbatimPolygonOrDie.") -std::unique_ptr MakeVerbatimPolygon(absl::string_view str); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakeVerbatimPolygon(absl::string_view str) { + return MakeVerbatimPolygonOrDie(str); +} // Parses a string in the same format as MakePolygon, except that loops must // be oriented so that the interior of the loop is always on the left, and @@ -227,8 +240,11 @@ std::unique_ptr MakeLaxPolygonOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool MakeLaxPolygon( absl::string_view str, std::unique_ptr* lax_polygon); -ABSL_DEPRECATED("Use MakeLaxPolygonOrDie.") -std::unique_ptr MakeLaxPolygon(absl::string_view str); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakeLaxPolygon( + absl::string_view str) { + return MakeLaxPolygonOrDie(str); +} // Returns a MutableS2ShapeIndex containing the points, polylines, and loops // (in the form of a single polygon) described by the following format: @@ -255,8 +271,10 @@ std::unique_ptr MakeIndexOrDie(absl::string_view str); ABSL_MUST_USE_RESULT bool MakeIndex( absl::string_view str, std::unique_ptr* index); -ABSL_DEPRECATED("Use MakeIndexOrDie.") -std::unique_ptr MakeIndex(absl::string_view str); +ABSL_DEPRECATED("Inline the implementation") +inline std::unique_ptr MakeIndex(absl::string_view str) { + return MakeIndexOrDie(str); +} // Convert an S2Point, S2LatLng, S2LatLngRect, S2CellId, S2CellUnion, loop, // polyline, or polygon to the string format above. diff --git a/src/s2/util/bits/bits.h b/src/s2/util/bits/bits.h index 4a80cb6a..19cf099d 100644 --- a/src/s2/util/bits/bits.h +++ b/src/s2/util/bits/bits.h @@ -78,8 +78,10 @@ class Bits { typedef typename UnsignedTypeBySize::Type Type; }; + ABSL_DEPRECATED("Inline the implementation") static int CountOnes(uint32 n) { return absl::popcount(n); } + ABSL_DEPRECATED("Inline the implementation") static inline int CountOnes64(uint64 n) { return absl::popcount(n); } // Count bits in uint128 @@ -117,7 +119,9 @@ class Bits { int num_bytes, int cap); // Return floor(log2(n)) for positive integer n. Returns -1 iff n == 0. + ABSL_DEPRECATED("Inline the implementation") static int Log2Floor(uint32 n); + ABSL_DEPRECATED("Inline the implementation") static int Log2Floor64(uint64 n); static int Log2Floor128(absl::uint128 n); @@ -135,6 +139,7 @@ class Bits { // Returns true if and only if n is a power of two. template ::value, int> = 0> + ABSL_DEPRECATED("Inline the implementation") static constexpr bool IsPowerOfTwo(IntType n) { return absl::has_single_bit(n); } From 211e4d0019dc111e90f576fd6a6dd56a61f836d6 Mon Sep 17 00:00:00 2001 From: Jesse Rosenstock Date: Wed, 16 Feb 2022 17:14:14 +0100 Subject: [PATCH 2/2] Restore C++11 compatibility --- CMakeLists.txt | 10 +++---- README.md | 9 +++++-- src/s2/base/casts.h | 6 ++--- src/s2/mutable_s2shape_index.h | 2 +- src/s2/r1interval.h | 11 +++++--- src/s2/s1interval.h | 1 + src/s2/s2boolean_operation.h | 3 ++- src/s2/s2boolean_operation_test.cc | 2 ++ src/s2/s2buffer_operation.cc | 5 ++++ src/s2/s2buffer_operation_test.cc | 7 +++-- src/s2/s2builder.cc | 1 + src/s2/s2builder.h | 2 ++ src/s2/s2builder_graph.cc | 1 + src/s2/s2builder_graph.h | 2 ++ src/s2/s2builder_graph_test.cc | 2 +- src/s2/s2builder_test.cc | 2 +- src/s2/s2builderutil_closed_set_normalizer.cc | 1 + src/s2/s2closest_cell_query_test.cc | 12 --------- src/s2/s2edge_distances.cc | 22 +++++++++------ src/s2/s2lax_polygon_shape.cc | 2 +- src/s2/s2lax_polygon_shape.h | 9 ++++--- src/s2/s2lax_polygon_shape_test.cc | 6 ++++- src/s2/s2lax_polyline_shape.h | 10 +++++-- src/s2/s2point_compression.cc | 2 ++ src/s2/s2point_vector_shape.h | 10 +++++-- src/s2/s2polygon.h | 7 +++-- src/s2/s2polyline.cc | 12 ++++++++- src/s2/s2polyline.h | 11 +++++++- src/s2/s2polyline_test.cc | 27 +++++++++++++++++++ src/s2/s2region_coverer_test.cc | 5 +++- src/s2/s2shape_index_region.h | 6 ++++- src/s2/thread_testing.cc | 1 + src/s2/util/coding/coder.cc | 23 +++++++++------- src/s2/util/coding/coder.h | 12 ++++++--- src/s2/util/math/exactfloat/exactfloat.cc | 2 +- src/s2/util/math/matrix3x3.h | 1 + src/s2/util/math/vector.h | 1 + src/s2/value_lexicon_test.cc | 2 ++ 38 files changed, 180 insertions(+), 70 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 648f8b61..0abd28a9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -62,14 +62,10 @@ endif() # s2geometry needs to use the same C++ standard that absl used to avoid # undefined symbol errors since ABSL_HAVE_STD_STRING_VIEW etc will # end up defined differently. There is probably a better way to achieve -# this than assuming what absl used. s2geometry should actually work fine -# with C++11, but there are a few places that use std::exchange instead of -# absl::exchange, is_x_t, and some atomic constructors that would need -# to change. File a bug if you need support for earlier versions. -# TODO(jrosenstock,b/205952836): Use absl functions if possible. -set(CMAKE_CXX_STANDARD 17) +# this than assuming what absl used. +set(CMAKE_CXX_STANDARD 11) set(CMAKE_CXX_STANDARD_REQUIRED ON) -# No compiler-specific extensions, i.e. -std=c++17, not -std=gnu++17. +# No compiler-specific extensions, i.e. -std=c++11, not -std=gnu++11. set(CMAKE_CXX_EXTENSIONS OFF) # Avoid megabytes of warnings like: diff --git a/README.md b/README.md index ed880e5b..8eb258ac 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ S2 documentation can be found on [s2geometry.io](http://s2geometry.io). ## Requirements for End Users * [CMake](http://www.cmake.org/) -* A C++ compiler with C++17 support, such as [g++ 7](https://gcc.gnu.org/) +* A C++ compiler with C++11 support, such as [g++ >= 4.7](https://gcc.gnu.org/) * [Abseil](https://github.com/abseil/abseil-cpp) (standard library extensions) * [OpenSSL](https://github.com/openssl/openssl) (for its bignum library) * [gflags command line flags](https://github.com/gflags/gflags), optional @@ -85,6 +85,10 @@ cd s2geometry First, [install Abseil](https://github.com/abseil/abseil-cpp/blob/master/CMake/README.md#traditional-cmake-set-up). It must be configured with `-DCMAKE_POSITION_INDEPENDENT_CODE=ON`. +s2geometry must be configured to use the came C++ version that +abseil uses. The easiest way to achieve this is to pass +`-DCMAKE_CXX_STANDARD=11` (or `-DCMAKE_CXX_STANDARD=17`) to `cmake` +when compiling both abseil and s2geometry. From the appropriate directory depending on how you got the source: @@ -92,7 +96,8 @@ From the appropriate directory depending on how you got the source: mkdir build cd build # You can omit -DGTEST_ROOT to skip tests; see above for macOS. -cmake -DGTEST_ROOT=/usr/src/gtest -DCMAKE_PREFIX_PATH=/path/to/absl/install .. +# Use the same CMAKE_CXX_STANDARD value that was used with absl. +cmake -DGTEST_ROOT=/usr/src/gtest -DCMAKE_PREFIX_PATH=/path/to/absl/install -DCMAKE_CXX_STANDARD=11 .. make -j $(nproc) make test ARGS="-j$(nproc)" # If GTEST_ROOT specified above. sudo make install diff --git a/src/s2/base/casts.h b/src/s2/base/casts.h index 65579eb1..22070e52 100644 --- a/src/s2/base/casts.h +++ b/src/s2/base/casts.h @@ -58,7 +58,7 @@ template // use like this: down_cast(foo); inline To down_cast(From* f) { // so we only accept pointers - static_assert((std::is_base_of>::value), + static_assert((std::is_base_of>::value), "target type not derived from source type"); // We skip the assert and hence the dynamic_cast if RTTI is disabled. @@ -82,13 +82,13 @@ template inline To down_cast(From& f) { static_assert(std::is_lvalue_reference::value, "target type not a reference"); - static_assert((std::is_base_of>::value), + static_assert((std::is_base_of>::value), "target type not derived from source type"); // We skip the assert and hence the dynamic_cast if RTTI is disabled. #if !defined(__GNUC__) || defined(__GXX_RTTI) // RTTI: debug mode only - assert(dynamic_cast*>(&f) != nullptr); + assert(dynamic_cast*>(&f) != nullptr); #endif // !defined(__GNUC__) || defined(__GXX_RTTI) return static_cast(f); diff --git a/src/s2/mutable_s2shape_index.h b/src/s2/mutable_s2shape_index.h index be8991a1..38045456 100644 --- a/src/s2/mutable_s2shape_index.h +++ b/src/s2/mutable_s2shape_index.h @@ -541,7 +541,7 @@ class MutableS2ShapeIndex final : public S2ShapeIndex { FRESH, // There are no pending updates. }; // Reads and writes to this field are guarded by "lock_". - std::atomic index_status_ = FRESH; + std::atomic index_status_{FRESH}; // UpdateState holds temporary data related to thread synchronization. It // is only allocated while updates are being applied. diff --git a/src/s2/r1interval.h b/src/s2/r1interval.h index ba0b8c26..b670b5c1 100644 --- a/src/s2/r1interval.h +++ b/src/s2/r1interval.h @@ -148,9 +148,14 @@ class R1Interval { // Expand the interval so that it contains the given point "p". void AddPoint(double p) { - if (is_empty()) { set_lo(p); set_hi(p); } - else if (p < lo()) { set_lo(p); } // NOLINT - else if (p > hi()) { set_hi(p); } // NOLINT + if (is_empty()) { + set_lo(p); + set_hi(p); + } else if (p < lo()) { + set_lo(p); + } else if (p > hi()) { + set_hi(p); + } } // Expand the interval so that it contains the given interval "y". diff --git a/src/s2/s1interval.h b/src/s2/s1interval.h index 74c7885d..b11b2baf 100644 --- a/src/s2/s1interval.h +++ b/src/s2/s1interval.h @@ -21,6 +21,7 @@ #include #include #include +#include #include "s2/base/logging.h" #include "s2/_fp_contract_off.h" diff --git a/src/s2/s2boolean_operation.h b/src/s2/s2boolean_operation.h index 5d22ff05..e04266cc 100644 --- a/src/s2/s2boolean_operation.h +++ b/src/s2/s2boolean_operation.h @@ -21,6 +21,7 @@ #include #include #include + #include "s2/s2builder.h" #include "s2/s2builder_graph.h" #include "s2/s2builder_layer.h" @@ -452,7 +453,7 @@ class S2BooleanOperation { private: std::unique_ptr snap_function_; - PolygonModel polygon_model_ = PolygonModel::SEMI_OPEN;; + PolygonModel polygon_model_ = PolygonModel::SEMI_OPEN; PolylineModel polyline_model_ = PolylineModel::CLOSED; bool polyline_loops_have_boundaries_ = true; bool split_all_crossing_polyline_edges_ = false; diff --git a/src/s2/s2boolean_operation_test.cc b/src/s2/s2boolean_operation_test.cc index 9892504d..931a8c03 100644 --- a/src/s2/s2boolean_operation_test.cc +++ b/src/s2/s2boolean_operation_test.cc @@ -18,7 +18,9 @@ #include "s2/s2boolean_operation.h" #include +#include #include +#include #include #include "absl/flags/reflection.h" diff --git a/src/s2/s2buffer_operation.cc b/src/s2/s2buffer_operation.cc index 91959c80..ab053f9c 100644 --- a/src/s2/s2buffer_operation.cc +++ b/src/s2/s2buffer_operation.cc @@ -51,6 +51,7 @@ #include #include #include +#include #include #include "absl/memory/memory.h" @@ -128,6 +129,10 @@ static constexpr S1Angle kMinRequestedError = S1Angle::Radians(2 * DBL_ERR); static constexpr S1Angle kMaxAbsoluteInterpolationError = S2::kGetPointOnLineError + S2::kGetPointOnRayPerpendicularError; +// TODO(user, b/210097200): Remove when we require c++17 for opensource. +constexpr double S2BufferOperation::Options::kMinErrorFraction; +constexpr double S2BufferOperation::Options::kMaxCircleSegments; + S2BufferOperation::Options::Options() : snap_function_( make_unique(S1Angle::Zero())) { diff --git a/src/s2/s2buffer_operation_test.cc b/src/s2/s2buffer_operation_test.cc index a0709a25..76fcac48 100644 --- a/src/s2/s2buffer_operation_test.cc +++ b/src/s2/s2buffer_operation_test.cc @@ -18,8 +18,11 @@ #include "s2/s2buffer_operation.h" #include +#include #include #include +#include +#include #include "s2/base/casts.h" #include "s2/base/logging.h" @@ -288,8 +291,8 @@ void TestMinimumDistance(const MutableS2ShapeIndex& input, S2ClosestEdgeQuery::EdgeTarget in_target(a.v0, a.v1); for (const auto& out_result : out_query.FindClosestEdges(&in_target)) { auto b = output.shape(out_result.shape_id())->edge(out_result.edge_id()); - ASSERT_GE(s2pred::CompareEdgePairDistance( - a.v0, a.v1, b.v0, b.v1, min_dist), 0);; + ASSERT_GE( + s2pred::CompareEdgePairDistance(a.v0, a.v1, b.v0, b.v1, min_dist), 0); } } } diff --git a/src/s2/s2builder.cc b/src/s2/s2builder.cc index e2b355ff..4b443643 100644 --- a/src/s2/s2builder.cc +++ b/src/s2/s2builder.cc @@ -74,6 +74,7 @@ #include #include #include +#include #include #include "s2/base/casts.h" diff --git a/src/s2/s2builder.h b/src/s2/s2builder.h index d76ad944..96aa7f4c 100644 --- a/src/s2/s2builder.h +++ b/src/s2/s2builder.h @@ -21,6 +21,8 @@ #ifndef S2_S2BUILDER_H_ #define S2_S2BUILDER_H_ +#include +#include #include #include #include diff --git a/src/s2/s2builder_graph.cc b/src/s2/s2builder_graph.cc index 26448916..68162ee7 100644 --- a/src/s2/s2builder_graph.cc +++ b/src/s2/s2builder_graph.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "s2/base/logging.h" diff --git a/src/s2/s2builder_graph.h b/src/s2/s2builder_graph.h index a919502d..767e2482 100644 --- a/src/s2/s2builder_graph.h +++ b/src/s2/s2builder_graph.h @@ -18,9 +18,11 @@ #ifndef S2_S2BUILDER_GRAPH_H_ #define S2_S2BUILDER_GRAPH_H_ +#include #include #include #include +#include #include #include diff --git a/src/s2/s2builder_graph_test.cc b/src/s2/s2builder_graph_test.cc index 45b1402f..1bfd00f1 100644 --- a/src/s2/s2builder_graph_test.cc +++ b/src/s2/s2builder_graph_test.cc @@ -65,7 +65,7 @@ TEST(Graph, LabelsRequestedButNotProvided) { vector vertices{S2Point(1, 0, 0)}; vector edges{{0, 0}}; vector input_edge_id_set_ids{0}; - IdSetLexicon input_edge_id_set_lexicon, label_set_lexicon;; + IdSetLexicon input_edge_id_set_lexicon, label_set_lexicon; vector label_set_ids; // Empty means no labels are present. Graph g{ options, &vertices, &edges, &input_edge_id_set_ids, diff --git a/src/s2/s2builder_test.cc b/src/s2/s2builder_test.cc index 12f2ec25..f4b59e4b 100644 --- a/src/s2/s2builder_test.cc +++ b/src/s2/s2builder_test.cc @@ -82,7 +82,7 @@ using s2textformat::MakePolylineOrDie; using EdgeType = S2Builder::EdgeType; using InputEdgeId = S2Builder::Graph::InputEdgeId; using Graph = S2Builder::Graph; -using GraphOptions = S2Builder::GraphOptions;; +using GraphOptions = S2Builder::GraphOptions; DEFINE_int32(iteration_multiplier, 1, "Iteration multiplier for randomized tests"); diff --git a/src/s2/s2builderutil_closed_set_normalizer.cc b/src/s2/s2builderutil_closed_set_normalizer.cc index 8be85f72..dc1965b6 100644 --- a/src/s2/s2builderutil_closed_set_normalizer.cc +++ b/src/s2/s2builderutil_closed_set_normalizer.cc @@ -18,6 +18,7 @@ #include "s2/s2builderutil_closed_set_normalizer.h" #include +#include #include "absl/memory/memory.h" #include "s2/s2builder_layer.h" diff --git a/src/s2/s2closest_cell_query_test.cc b/src/s2/s2closest_cell_query_test.cc index db3934b6..0367cf6a 100644 --- a/src/s2/s2closest_cell_query_test.cc +++ b/src/s2/s2closest_cell_query_test.cc @@ -220,18 +220,6 @@ static const S1Angle kTestCapRadius = S2Testing::KmToAngle(10); using TestingResult = pair; -// Converts to the format required by CheckDistanceResults() in s2testing.h. -vector ConvertResults( - const vector& results) { - vector testing_results; - for (const auto& result : results) { - testing_results.push_back( - TestingResult(result.distance(), - LabelledCell(result.cell_id(), result.label()))); - } - return testing_results; -} - // Use "query" to find the closest cell(s) to the given target, and extract // the query results into the given vector. Also verify that the results // satisfy the search criteria. diff --git a/src/s2/s2edge_distances.cc b/src/s2/s2edge_distances.cc index eeebdbba..20ef1aab 100644 --- a/src/s2/s2edge_distances.cc +++ b/src/s2/s2edge_distances.cc @@ -334,10 +334,13 @@ bool UpdateEdgePairMinDistance( // // The calculation below computes each of the six vertex-vertex distances // twice (this could be optimized). - return (UpdateMinDistance(a0, b0, b1, min_dist) | - UpdateMinDistance(a1, b0, b1, min_dist) | - UpdateMinDistance(b0, a0, a1, min_dist) | - UpdateMinDistance(b1, a0, a1, min_dist)); + // + // We do not want the short circuit behavior of ||. Suppress + // -Wbitwise-instead-of-logical errors by converting to int. + return (int{UpdateMinDistance(a0, b0, b1, min_dist)} | + int{UpdateMinDistance(a1, b0, b1, min_dist)} | + int{UpdateMinDistance(b0, a0, a1, min_dist)} | + int{UpdateMinDistance(b1, a0, a1, min_dist)}); } bool UpdateEdgePairMaxDistance( @@ -357,10 +360,13 @@ bool UpdateEdgePairMaxDistance( // // The calculation below computes each of the six vertex-vertex distances // twice (this could be optimized). - return (UpdateMaxDistance(a0, b0, b1, max_dist) | - UpdateMaxDistance(a1, b0, b1, max_dist) | - UpdateMaxDistance(b0, a0, a1, max_dist) | - UpdateMaxDistance(b1, a0, a1, max_dist)); + // + // We do not want the short circuit behavior of ||. Suppress + // -Wbitwise-instead-of-logical errors by converting to int. + return (int{UpdateMaxDistance(a0, b0, b1, max_dist)} | + int{UpdateMaxDistance(a1, b0, b1, max_dist)} | + int{UpdateMaxDistance(b0, a0, a1, max_dist)} | + int{UpdateMaxDistance(b1, a0, a1, max_dist)}); } std::pair GetEdgePairClosestPoints( diff --git a/src/s2/s2lax_polygon_shape.cc b/src/s2/s2lax_polygon_shape.cc index 1e6b5bb9..e06e88dd 100644 --- a/src/s2/s2lax_polygon_shape.cc +++ b/src/s2/s2lax_polygon_shape.cc @@ -32,7 +32,7 @@ namespace { template std::unique_ptr make_unique_for_overwrite(size_t n) { // We only need to support this one variant. - static_assert(std::is_array_v); + static_assert(std::is_array::value); return std::unique_ptr(new typename absl::remove_extent_t[n]); } } // namespace diff --git a/src/s2/s2lax_polygon_shape.h b/src/s2/s2lax_polygon_shape.h index 96b0a0cf..d21eaf43 100644 --- a/src/s2/s2lax_polygon_shape.h +++ b/src/s2/s2lax_polygon_shape.h @@ -75,7 +75,10 @@ // (S2BooleanOperation, S2ClosestEdgeQuery, etc). class S2LaxPolygonShape : public S2Shape { public: - static constexpr TypeTag kTypeTag = 5; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is allowed + // in opensource. + enum : TypeTag { kTypeTag = 5 }; // Constructs an empty polygon. S2LaxPolygonShape() : num_loops_(0), num_vertices_(0) {} @@ -148,7 +151,7 @@ class S2LaxPolygonShape : public S2Shape { // The loop that contained the edge returned by the previous call to the // edge() method. This is used as a hint to speed up edge location when // there are many loops. - mutable std::atomic prev_loop_ = 0; + mutable std::atomic prev_loop_{0}; int32 num_vertices_; std::unique_ptr vertices_; @@ -205,7 +208,7 @@ class EncodedS2LaxPolygonShape : public S2Shape { // The loop that contained the edge returned by the previous call to the // edge() method. This is used as a hint to speed up edge location when // there are many loops. - mutable std::atomic prev_loop_ = 0; + mutable std::atomic prev_loop_{0}; s2coding::EncodedS2PointVector vertices_; s2coding::EncodedUintVector loop_starts_; diff --git a/src/s2/s2lax_polygon_shape_test.cc b/src/s2/s2lax_polygon_shape_test.cc index ed573792..091a3655 100644 --- a/src/s2/s2lax_polygon_shape_test.cc +++ b/src/s2/s2lax_polygon_shape_test.cc @@ -248,7 +248,11 @@ TEST(S2LaxPolygonShape, ManyLoopPolygon) { } } std::shuffle(edges.begin(), edges.end(), std::mt19937_64()); - for (const auto [e, i, j] : edges) { + // TODO(user,b/210097200): Use structured bindings when we require + // C++17 in opensource. + for (const auto t : edges) { + int e, i, j; + std::tie(e, i, j) = t; EXPECT_EQ(shape.chain_position(e), S2Shape::ChainPosition(i, j)); auto v0 = loops[i][j]; auto v1 = loops[i][(j + 1) % loops[i].size()]; diff --git a/src/s2/s2lax_polyline_shape.h b/src/s2/s2lax_polyline_shape.h index e97159fa..17f85086 100644 --- a/src/s2/s2lax_polyline_shape.h +++ b/src/s2/s2lax_polyline_shape.h @@ -37,7 +37,10 @@ // or use S2LaxClosedPolylineShape defined in s2_lax_loop_shape.h.) class S2LaxPolylineShape : public S2Shape { public: - static constexpr TypeTag kTypeTag = 4; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is allowed + // in opensource. + enum : TypeTag { kTypeTag = 4 }; // Constructs an empty polyline. S2LaxPolylineShape() : num_vertices_(0) {} @@ -102,7 +105,10 @@ class S2LaxPolylineShape : public S2Shape { // into a large contiguous buffer that contains other encoded data as well. class EncodedS2LaxPolylineShape : public S2Shape { public: - static constexpr TypeTag kTypeTag = S2LaxPolylineShape::kTypeTag; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is allowed + // in opensource. + enum : TypeTag { kTypeTag = S2LaxPolylineShape::kTypeTag }; // Constructs an uninitialized object; requires Init() to be called. EncodedS2LaxPolylineShape() {} diff --git a/src/s2/s2point_compression.cc b/src/s2/s2point_compression.cc index c8aec9df..dde5321b 100644 --- a/src/s2/s2point_compression.cc +++ b/src/s2/s2point_compression.cc @@ -344,6 +344,8 @@ void S2EncodePointsCompressed(Span points, bool S2DecodePointsCompressed(Decoder* decoder, int level, Span points) { + S2_DCHECK_LE(level, S2::kMaxCellLevel); + Faces faces; if (!faces.Decode(points.size(), decoder)) { return false; diff --git a/src/s2/s2point_vector_shape.h b/src/s2/s2point_vector_shape.h index 3c33a2e2..f58c0e97 100644 --- a/src/s2/s2point_vector_shape.h +++ b/src/s2/s2point_vector_shape.h @@ -29,7 +29,10 @@ // This class is useful for adding a collection of points to an S2ShapeIndex. class S2PointVectorShape : public S2Shape { public: - static constexpr TypeTag kTypeTag = 3; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is allowed + // in opensource. + enum : TypeTag { kTypeTag = 3 }; // Constructs an empty point vector. S2PointVectorShape() {} @@ -90,7 +93,10 @@ class S2PointVectorShape : public S2Shape { // into a large contiguous buffer that contains other encoded data as well. class EncodedS2PointVectorShape : public S2Shape { public: - static constexpr TypeTag kTypeTag = S2PointVectorShape::kTypeTag; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is allowed + // in opensource. + enum : TypeTag { kTypeTag = S2PointVectorShape::kTypeTag }; // Constructs an uninitialized object; requires Init() to be called. EncodedS2PointVectorShape() {} diff --git a/src/s2/s2polygon.h b/src/s2/s2polygon.h index fa98bf03..2dca5143 100644 --- a/src/s2/s2polygon.h +++ b/src/s2/s2polygon.h @@ -717,7 +717,10 @@ class S2Polygon final : public S2Region { // such that the polygon interior is always on the left. class Shape : public S2Shape { public: - static constexpr TypeTag kTypeTag = 1; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is + // allowed in opensource. + enum : TypeTag { kTypeTag = 1 }; Shape() : polygon_(nullptr), loop_starts_(nullptr) {} ~Shape() override; @@ -758,7 +761,7 @@ class S2Polygon final : public S2Region { // edge() method. This is used as a hint to speed up edge location when // there are many loops. Note that this field does not take up any space // due to field packing with S2Shape::id_. - mutable std::atomic prev_loop_ = 0; + mutable std::atomic prev_loop_{0}; const S2Polygon* polygon_; diff --git a/src/s2/s2polyline.cc b/src/s2/s2polyline.cc index 245c91bb..c785f74a 100644 --- a/src/s2/s2polyline.cc +++ b/src/s2/s2polyline.cc @@ -41,6 +41,7 @@ #include "s2/s2edge_distances.h" #include "s2/s2error.h" #include "s2/s2latlng_rect_bounder.h" +#include "s2/s2point.h" #include "s2/s2point_compression.h" #include "s2/s2pointutil.h" #include "s2/s2polyline_measures.h" @@ -431,8 +432,11 @@ bool S2Polyline::DecodeUncompressed(Decoder* const decoder) { return false; } num_vertices_ = decoder->get32(); + + // Check the bytes available before allocating memory in case of + // corrupt/malicious input. + if (decoder->avail() < num_vertices_ * sizeof(S2Point)) return false; vertices_.reset(new S2Point[num_vertices_]); - if (decoder->avail() < num_vertices_ * sizeof(vertices_[0])) return false; decoder->getn(&vertices_[0], num_vertices_ * sizeof(vertices_[0])); if (absl::GetFlag(FLAGS_s2debug) && s2debug_override_ == S2Debug::ALLOW) { @@ -505,6 +509,8 @@ void S2Polyline::EncodeCompressed(Encoder* encoder, bool S2Polyline::DecodeCompressed(Decoder* decoder) { if (decoder->avail() < sizeof(uint8)) return false; const int snap_level = decoder->get8(); + if (snap_level > S2::kMaxCellLevel) return false; + vector points; uint32 num_vertices; if (!decoder->get_varint32(&num_vertices)) return false; @@ -513,6 +519,10 @@ bool S2Polyline::DecodeCompressed(Decoder* decoder) { Init(points); return true; } + + // TODO(b/209937354): Prevent large allocations like in DecodeUncompressed. + // This is more complicated due to the compressed encoding, but perhaps the + // minimum required size can be bounded. points.resize(num_vertices); if (!S2DecodePointsCompressed(decoder, snap_level, absl::MakeSpan(points))) { return false; diff --git a/src/s2/s2polyline.h b/src/s2/s2polyline.h index c33f2519..e9395b48 100644 --- a/src/s2/s2polyline.h +++ b/src/s2/s2polyline.h @@ -32,6 +32,7 @@ #include "s2/s2debug.h" #include "s2/s2error.h" #include "s2/s2latlng_rect.h" +#include "s2/s2point_span.h" #include "s2/s2region.h" #include "s2/s2shape.h" @@ -136,6 +137,11 @@ class S2Polyline final : public S2Region { return vertices_[k]; } + // Returns an S2PointSpan containing the polyline's vertices. + S2PointSpan vertices_span() const { + return S2PointSpan(vertices_.get(), num_vertices_); + } + // Return the length of the polyline. S1Angle GetLength() const; @@ -318,7 +324,10 @@ class S2Polyline final : public S2Region { // data (see S2Shape for details). class Shape : public S2Shape { public: - static constexpr TypeTag kTypeTag = 2; + // Define as enum so we don't have to declare storage. + // TODO(user, b/210097200): Use static constexpr when C++17 is + // allowed in opensource. + enum : TypeTag { kTypeTag = 2 }; Shape() : polyline_(nullptr) {} // Must call Init(). diff --git a/src/s2/s2polyline_test.cc b/src/s2/s2polyline_test.cc index 3af786f1..792ceb8d 100644 --- a/src/s2/s2polyline_test.cc +++ b/src/s2/s2polyline_test.cc @@ -31,6 +31,7 @@ #include "s2/s1angle.h" #include "s2/s2builderutil_snap_functions.h" #include "s2/s2cell.h" +#include "s2/s2coords.h" #include "s2/s2debug.h" #include "s2/s2latlng.h" #include "s2/s2pointutil.h" @@ -76,6 +77,8 @@ TEST(S2Polyline, Basic) { S2Point(0, 1, 0))); semi_equator.Reverse(); EXPECT_EQ(S2Point(1, 0, 0), semi_equator.vertex(2)); + ASSERT_EQ(3, semi_equator.vertices_span().size()); + EXPECT_EQ(S2Point(1, 0, 0), semi_equator.vertices_span()[2]); } TEST(S2Polyline, NoData) { @@ -532,6 +535,30 @@ TEST(S2Polyline, DecodeCompressedBadData) { EXPECT_FALSE(decoded_polyline.Decode(&decoder)); } +TEST(S2Polyline, DecodeCompressedMaxCellLevel) { + Encoder encoder; + encoder.Ensure(6); + encoder.put8(2); // kCurrentCompressedEncodingVersionNumber + encoder.put8(S2::kMaxCellLevel); + encoder.put32(0); + Decoder decoder(encoder.base(), encoder.length()); + + S2Polyline polyline; + EXPECT_TRUE(polyline.Decode(&decoder)); +} + +TEST(S2Polyline, DecodeCompressedCellLevelTooHigh) { + Encoder encoder; + encoder.Ensure(6); + encoder.put8(2); // kCurrentCompressedEncodingVersionNumber + encoder.put8(S2::kMaxCellLevel + 1); + encoder.put32(0); + Decoder decoder(encoder.base(), encoder.length()); + + S2Polyline polyline; + EXPECT_FALSE(polyline.Decode(&decoder)); +} + TEST(S2PolylineShape, Basic) { unique_ptr polyline(MakePolyline("0:0, 1:0, 1:1, 2:1")); S2Polyline::Shape shape(polyline.get()); diff --git a/src/s2/s2region_coverer_test.cc b/src/s2/s2region_coverer_test.cc index d836aa31..72f0ab14 100644 --- a/src/s2/s2region_coverer_test.cc +++ b/src/s2/s2region_coverer_test.cc @@ -95,7 +95,10 @@ static void CheckCovering(const S2RegionCoverer::Options& options, if (covering.size() > options.max_cells()) { // If the covering has more than the requested number of cells, then check // that the cell count cannot be reduced by using the parent of some cell. - for (const auto [_, cells] : min_level_cells) { + // TODO(user,b/210097200): Use structured bindings when we require + // C++17 in opensource. + for (const auto& p : min_level_cells) { + const int cells = p.second; EXPECT_EQ(cells, 1); } } diff --git a/src/s2/s2shape_index_region.h b/src/s2/s2shape_index_region.h index 7b90e183..eee0dacd 100644 --- a/src/s2/s2shape_index_region.h +++ b/src/s2/s2shape_index_region.h @@ -360,7 +360,11 @@ bool S2ShapeIndexRegion::VisitIntersectingShapes( clipped.num_edges() > 0 || !clipped.contains_center(); } } - for (const auto& [shape_id, not_contains] : shape_not_contains) { + // TODO(user,b/210097200): Use structured bindings when we require + // C++17 in opensource. + for (const auto& p : shape_not_contains) { + const int shape_id = p.first; + const bool not_contains = p.second; if (!visitor(index().shape(shape_id), !not_contains)) return false; } return true; diff --git a/src/s2/thread_testing.cc b/src/s2/thread_testing.cc index 4df547f7..472e3843 100644 --- a/src/s2/thread_testing.cc +++ b/src/s2/thread_testing.cc @@ -17,6 +17,7 @@ #include "s2/thread_testing.h" +#include #include #include diff --git a/src/s2/util/coding/coder.cc b/src/s2/util/coding/coder.cc index ab5841f0..1bae3929 100644 --- a/src/s2/util/coding/coder.cc +++ b/src/s2/util/coding/coder.cc @@ -27,20 +27,21 @@ #include "s2/base/integral_types.h" #include "s2/base/logging.h" #include "s2/base/port.h" +#include "absl/utility/utility.h" Encoder::Encoder(Encoder&& other) - : buf_(std::exchange(other.buf_, nullptr)), - limit_(std::exchange(other.limit_, nullptr)), - underlying_buffer_(std::exchange(other.underlying_buffer_, nullptr)), - orig_(std::exchange(other.orig_, nullptr)) {} + : buf_(absl::exchange(other.buf_, nullptr)), + limit_(absl::exchange(other.limit_, nullptr)), + underlying_buffer_(absl::exchange(other.underlying_buffer_, nullptr)), + orig_(absl::exchange(other.orig_, nullptr)) {} Encoder& Encoder::operator=(Encoder&& other) { if (this == &other) return *this; if (ensure_allowed()) DeleteBuffer(underlying_buffer_, capacity()); - buf_ = std::exchange(other.buf_, nullptr); - limit_ = std::exchange(other.limit_, nullptr); - underlying_buffer_ = std::exchange(other.underlying_buffer_, nullptr); - orig_ = std::exchange(other.orig_, nullptr); + buf_ = absl::exchange(other.buf_, nullptr); + limit_ = absl::exchange(other.limit_, nullptr); + underlying_buffer_ = absl::exchange(other.underlying_buffer_, nullptr); + orig_ = absl::exchange(other.orig_, nullptr); return *this; } @@ -69,7 +70,11 @@ void Encoder::EnsureSlowPath(size_t N) { // Double buffer size, but make sure we always have at least N extra bytes const size_t current_len = length(); - const auto [new_buffer, new_capacity] = + // Used in opensource; avoid structured bindings (a C++17 feature) to + // remain C++11-compatible. b/210097200 + unsigned char* new_buffer; + size_t new_capacity; + std::tie(new_buffer, new_capacity) = NewBuffer(std::max(current_len + N, 2 * current_len)); if (underlying_buffer_) { diff --git a/src/s2/util/coding/coder.h b/src/s2/util/coding/coder.h index b30a42d3..2559e63a 100644 --- a/src/s2/util/coding/coder.h +++ b/src/s2/util/coding/coder.h @@ -34,6 +34,7 @@ #include "absl/base/macros.h" #include "absl/meta/type_traits.h" #include "absl/numeric/int128.h" +#include "absl/utility/utility.h" #include "s2/util/coding/varint.h" #include "s2/util/endian/endian.h" @@ -141,10 +142,13 @@ class Encoder { private: // All encoding operations are done through the Writer. This avoids aliasing // between `buf_` and `this` which allows the compiler to avoid reloading - // `buf_` repeatedly. See https://godbolt.org/z/sfEnePYWP. + // `buf_` repeatedly. See https://godbolt.org/z/zM36s3ded. struct Writer { Encoder* enc; - char* p{reinterpret_cast(enc->buf_)}; + char* p; + + explicit Writer(Encoder* e) + : enc(e), p(reinterpret_cast(enc->buf_)) {} ~Writer() { enc->buf_ = reinterpret_cast(p); @@ -152,7 +156,7 @@ class Encoder { S2_DCHECK_LE(enc->buf_, enc->limit_); } - char* skip(ptrdiff_t N) { return std::exchange(p, p + N); } + char* skip(ptrdiff_t N) { return absl::exchange(p, p + N); } void put8(unsigned char v) { *p++ = v; } void put16(uint16 v) { LittleEndian::Store16(skip(2), v); } @@ -181,7 +185,7 @@ class Encoder { } }; - Writer writer() { return Writer{this}; } + Writer writer() { return Writer(this); } static std::pair NewBuffer(size_t size); static void DeleteBuffer(unsigned char* buf, size_t size); diff --git a/src/s2/util/math/exactfloat/exactfloat.cc b/src/s2/util/math/exactfloat/exactfloat.cc index a8da9579..8f6365b9 100644 --- a/src/s2/util/math/exactfloat/exactfloat.cc +++ b/src/s2/util/math/exactfloat/exactfloat.cc @@ -400,7 +400,7 @@ std::string ExactFloat::ToStringWithMaxDigits(int max_digits) const { // Use fixed format. We split this into two cases depending on whether // the integer portion is non-zero or not. if (exp10 > 0) { - if (exp10 >= digits.size()) { + if (static_cast(exp10) >= digits.size()) { str += digits; for (int i = exp10 - digits.size(); i > 0; --i) { str.push_back('0'); diff --git a/src/s2/util/math/matrix3x3.h b/src/s2/util/math/matrix3x3.h index b2265039..24282503 100644 --- a/src/s2/util/math/matrix3x3.h +++ b/src/s2/util/math/matrix3x3.h @@ -33,6 +33,7 @@ #include #include +#include #include "s2/base/logging.h" #include "s2/util/math/mathutil.h" diff --git a/src/s2/util/math/vector.h b/src/s2/util/math/vector.h index 7bb80ccf..32e29c57 100644 --- a/src/s2/util/math/vector.h +++ b/src/s2/util/math/vector.h @@ -28,6 +28,7 @@ #include #include // NOLINT(readability/streams) #include +#include #include #include "s2/base/integral_types.h" diff --git a/src/s2/value_lexicon_test.cc b/src/s2/value_lexicon_test.cc index 28c1d5fb..a6d29d2a 100644 --- a/src/s2/value_lexicon_test.cc +++ b/src/s2/value_lexicon_test.cc @@ -17,7 +17,9 @@ #include "s2/value_lexicon.h" +#include #include +#include #include #include "absl/memory/memory.h"