Merge pull request #229 from jmr/update-2021-12-22

Drop required C++ version to C++11

CMakeLists.txt

@@ -18,14 +18,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)
 
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/third_party/cmake")

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,14 +85,19 @@ 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:
 
 ```
 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

src/s2/base/casts.h

@@ -58,7 +58,7 @@
 
 template <typename To, typename From>  // use like this: down_cast<T*>(foo);
 inline To down_cast(From* f) {         // so we only accept pointers
-  static_assert((std::is_base_of<From, std::remove_pointer_t<To>>::value),
+  static_assert((std::is_base_of<From, absl::remove_pointer_t<To>>::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 <typename To, typename From>
 inline To down_cast(From& f) {
   static_assert(std::is_lvalue_reference<To>::value,
                 "target type not a reference");
-  static_assert((std::is_base_of<From, std::remove_reference_t<To>>::value),
+  static_assert((std::is_base_of<From, absl::remove_reference_t<To>>::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<std::remove_reference_t<To>*>(&f) != nullptr);
+  assert(dynamic_cast<absl::remove_reference_t<To>*>(&f) != nullptr);
 #endif  // !defined(__GNUC__) || defined(__GXX_RTTI)
 
   return static_cast<To>(f);

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<IndexStatus> index_status_ = FRESH;
+  std::atomic<IndexStatus> index_status_{FRESH};
 
   // UpdateState holds temporary data related to thread synchronization.  It
   // is only allocated while updates are being applied.

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".

src/s2/s1interval.h

@@ -21,6 +21,7 @@
 #include <cmath>
 #include <iosfwd>
 #include <iostream>
+#include <ostream>
 
 #include "s2/base/logging.h"
 #include "s2/_fp_contract_off.h"

src/s2/s2boolean_operation.h

@@ -21,6 +21,7 @@
 #include <memory>
 #include <utility>
 #include <vector>
+
 #include "s2/s2builder.h"
 #include "s2/s2builder_graph.h"
 #include "s2/s2builder_layer.h"
@@ -452,7 +453,7 @@ class S2BooleanOperation {
 
    private:
     std::unique_ptr<S2Builder::SnapFunction> 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;

src/s2/s2boolean_operation_test.cc

@@ -18,7 +18,9 @@
 #include "s2/s2boolean_operation.h"
 
 #include <memory>
+#include <string>
 #include <tuple>
+#include <utility>
 
 #include <gtest/gtest.h>
 

src/s2/s2buffer_operation.cc

@@ -51,6 +51,7 @@
 #include <algorithm>
 #include <cmath>
 #include <memory>
+#include <utility>
 #include <vector>
 
 #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<s2builderutil::IdentitySnapFunction>(S1Angle::Zero())) {

src/s2/s2buffer_operation_test.cc

@@ -18,8 +18,11 @@
 #include "s2/s2buffer_operation.h"
 
 #include <algorithm>
+#include <functional>
 #include <iostream>
 #include <memory>
+#include <string>
+#include <utility>
 
 #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);
     }
   }
 }

src/s2/s2builder.cc

@@ -74,6 +74,7 @@
 #include <iostream>
 #include <memory>
 #include <numeric>
+#include <utility>
 #include <vector>
 
 #include "absl/cleanup/cleanup.h"

src/s2/s2builder.h

@@ -21,6 +21,8 @@
 #ifndef S2_S2BUILDER_H_
 #define S2_S2BUILDER_H_
 
+#include <algorithm>
+#include <functional>
 #include <memory>
 #include <utility>
 #include <vector>

src/s2/s2builder_graph.cc

@@ -21,6 +21,7 @@
 #include <limits>
 #include <memory>
 #include <numeric>
+#include <utility>
 #include <vector>
 
 #include "s2/base/logging.h"

src/s2/s2builder_graph.h

@@ -18,9 +18,11 @@
 #ifndef S2_S2BUILDER_GRAPH_H_
 #define S2_S2BUILDER_GRAPH_H_
 
+#include <algorithm>
 #include <array>
 #include <cstddef>
 #include <iterator>
+#include <limits>
 #include <utility>
 #include <vector>
 

src/s2/s2builder_graph_test.cc

@@ -65,7 +65,7 @@ TEST(Graph, LabelsRequestedButNotProvided) {
   vector<S2Point> vertices{S2Point(1, 0, 0)};
   vector<Edge> edges{{0, 0}};
   vector<InputEdgeIdSetId> 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<LabelSetId> label_set_ids;  // Empty means no labels are present.
   Graph g{
     options, &vertices, &edges, &input_edge_id_set_ids,

src/s2/s2builder_test.cc

@@ -84,7 +84,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;
 
 S2_DEFINE_int32(iteration_multiplier, 1,
                 "Iteration multiplier for randomized tests");

src/s2/s2builderutil_closed_set_normalizer.cc

@@ -18,6 +18,7 @@
 #include "s2/s2builderutil_closed_set_normalizer.h"
 
 #include <memory>
+#include <utility>
 
 #include "absl/memory/memory.h"
 #include "s2/s2builder_layer.h"

src/s2/s2closest_cell_query_test.cc

@@ -222,18 +222,6 @@ static const S1Angle kTestCapRadius = S2Testing::KmToAngle(10);
 
 using TestingResult = pair<S2MinDistance, LabelledCell>;
 
-// Converts to the format required by CheckDistanceResults() in s2testing.h.
-vector<TestingResult> ConvertResults(
-    const vector<S2ClosestCellQuery::Result>& results) {
-  vector<TestingResult> 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.

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<S2Point, S2Point> GetEdgePairClosestPoints(

src/s2/s2lax_polygon_shape.cc

@@ -33,7 +33,7 @@ namespace {
 template <typename T>
 std::unique_ptr<T> make_unique_for_overwrite(size_t n) {
   // We only need to support this one variant.
-  static_assert(std::is_array_v<T>);
+  static_assert(std::is_array<T>::value);
   return std::unique_ptr<T>(new typename absl::remove_extent_t<T>[n]);
 }
 }  // namespace

src/s2/s2lax_polygon_shape.h

@@ -76,7 +76,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) {}
@@ -149,7 +152,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<int> prev_loop_ = 0;
+  mutable std::atomic<int> prev_loop_{0};
 
   int32 num_vertices_;
   std::unique_ptr<S2Point[]> vertices_;
@@ -206,7 +209,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<int> prev_loop_ = 0;
+  mutable std::atomic<int> prev_loop_{0};
 
   s2coding::EncodedS2PointVector vertices_;
   s2coding::EncodedUintVector<uint32> loop_starts_;

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()];

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() {}

src/s2/s2point_compression.cc

@@ -345,6 +345,8 @@ void S2EncodePointsCompressed(Span<const S2XYZFaceSiTi> points,
 
 bool S2DecodePointsCompressed(Decoder* decoder, int level,
                               Span<S2Point> points) {
+  S2_DCHECK_LE(level, S2::kMaxCellLevel);
+
   Faces faces;
   if (!faces.Decode(points.size(), decoder)) {
     return false;