cmake build fixes (firebase#770)

* Fix nanopb (in cmake build)

Look for binaries in the src dir (since that's where we build now.) This
error would be masked if a previous build had completed prior to
switching nanopb to build out of src.

Also, don't patch the protoc path multiple times. This could be
triggered by (eg) 'make && make clean && make'.

* Add resource_path.{h,cc} to the cmake build

* Fix signed/unsigned int comparison warnings

* Ensure FieldValue tag_ is initialized during cp/mv ctor.

Otherwise, the assignment operator attempts to deallocate based on the
(uninitialized) tag_ variable, posssibly leading to segfaults.

* Fix tests that throw exceptions.

The (previous) tests checked to ensure that an abort() occurs, but if
ABSL_HAVE_EXCEPTIONS is defined on non-macos (which is currently the
default) then the assertions will throw a std::logic_error rather than
abort()ing.

On macos, an exception is thrown too, but the exception doesn't derrive
from std::exception, so ASSERT_DEATH_* doesn't catch it (hence why
ASSERT_DEATH_* actually works.)

To resolve this, I've switched to ASSERT_ANY_THROW.

Firestore/core/src/firebase/firestore/model/CMakeLists.txt

@@ -22,6 +22,8 @@ cc_library(
     field_path.h
     field_value.cc
     field_value.h
+    resource_path.cc
+    resource_path.h
     timestamp.cc
     timestamp.h
   DEPENDS

Firestore/core/src/firebase/firestore/model/base_path.h

@@ -166,7 +166,7 @@ class BasePath {
   }
   BasePath(std::initializer_list<std::string> list) : segments_{list} {
   }
-  BasePath(SegmentsT&& segments) : segments_{std::move(segments)} {
+  explicit BasePath(SegmentsT&& segments) : segments_{std::move(segments)} {
   }
 
  private:

Firestore/core/src/firebase/firestore/model/field_path.cc

@@ -51,7 +51,7 @@ bool IsValidIdentifier(const std::string& segment) {
       (first < 'A' || first > 'Z')) {
     return false;
   }
-  for (int i = 1; i != segment.size(); ++i) {
+  for (size_t i = 1; i != segment.size(); ++i) {
     const unsigned char c = segment[i];
     if (c != '_' && (c < 'a' || c > 'z') && (c < 'A' || c > 'Z') &&
         (c < '0' || c > '9')) {
@@ -93,7 +93,7 @@ FieldPath FieldPath::FromServerFormat(const absl::string_view path) {
 
   // Inside backticks, dots are treated literally.
   bool inside_backticks = false;
-  int i = 0;
+  size_t i = 0;
   while (i < path.size()) {
     const char c = path[i];
     // std::string (and string_view) may contain embedded nulls. For full

Firestore/core/src/firebase/firestore/model/field_path.h

@@ -82,7 +82,7 @@ class FieldPath : public impl::BasePath<FieldPath> {
   }
 
  private:
-  FieldPath(SegmentsT&& segments) : BasePath{std::move(segments)} {
+  explicit FieldPath(SegmentsT&& segments) : BasePath{std::move(segments)} {
   }
 
   // So that methods of base can construct FieldPath using the private

Firestore/core/src/firebase/firestore/model/field_value.h

@@ -69,7 +69,7 @@ class FieldValue {
     // position instead, see the doc comment above.
   };
 
-  FieldValue() : tag_(Type::Null) {
+  FieldValue() {
   }
 
   // Do not inline these ctor/dtor below, which contain call to non-trivial
@@ -123,7 +123,7 @@ class FieldValue {
    */
   void SwitchTo(const Type type);
 
-  Type tag_;
+  Type tag_ = Type::Null;
   union {
     // There is no null type as tag_ alone is enough for Null FieldValue.
     bool boolean_value_;

Firestore/core/src/firebase/firestore/model/resource_path.cc

@@ -18,6 +18,7 @@
 
 #include <algorithm>
 #include <utility>
+#include <vector>
 
 #include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"
 #include "absl/strings/str_join.h"

Firestore/core/src/firebase/firestore/model/resource_path.h

@@ -19,6 +19,7 @@
 
 #include <initializer_list>
 #include <string>
+#include <utility>
 
 #include "Firestore/core/src/firebase/firestore/model/base_path.h"
 #include "absl/strings/string_view.h"
@@ -69,7 +70,7 @@ class ResourcePath : public impl::BasePath<ResourcePath> {
   }
 
  private:
-  ResourcePath(SegmentsT&& segments) : BasePath{std::move(segments)} {
+  explicit ResourcePath(SegmentsT&& segments) : BasePath{std::move(segments)} {
   }
 
   // So that methods of base can construct ResourcePath using the private
@@ -81,4 +82,4 @@ class ResourcePath : public impl::BasePath<ResourcePath> {
 }  // namespace firestore
 }  // namespace firebase
 
-#endif
+#endif  // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_RESOURCE_PATH_H_

Firestore/core/test/firebase/firestore/model/field_path_test.cc

@@ -29,18 +29,18 @@ namespace model {
 TEST(FieldPath, Constructors) {
   const FieldPath empty_path;
   EXPECT_TRUE(empty_path.empty());
-  EXPECT_EQ(0, empty_path.size());
+  EXPECT_EQ(0u, empty_path.size());
   EXPECT_TRUE(empty_path.begin() == empty_path.end());
 
   const FieldPath path_from_list = {"rooms", "Eros", "messages"};
   EXPECT_FALSE(path_from_list.empty());
-  EXPECT_EQ(3, path_from_list.size());
+  EXPECT_EQ(3u, path_from_list.size());
   EXPECT_TRUE(path_from_list.begin() + 3 == path_from_list.end());
 
   std::vector<std::string> segments{"rooms", "Eros", "messages"};
   const FieldPath path_from_segments{segments.begin(), segments.end()};
   EXPECT_FALSE(path_from_segments.empty());
-  EXPECT_EQ(3, path_from_segments.size());
+  EXPECT_EQ(3u, path_from_segments.size());
   EXPECT_TRUE(path_from_segments.begin() + 3 == path_from_segments.end());
 
   FieldPath copied = path_from_list;
@@ -168,13 +168,13 @@ TEST(FieldPath, IsPrefixOf) {
 
 TEST(FieldPath, AccessFailures) {
   const FieldPath path;
-  ASSERT_DEATH_IF_SUPPORTED(path.first_segment(), "");
-  ASSERT_DEATH_IF_SUPPORTED(path.last_segment(), "");
-  ASSERT_DEATH_IF_SUPPORTED(path[0], "");
-  ASSERT_DEATH_IF_SUPPORTED(path[1], "");
-  ASSERT_DEATH_IF_SUPPORTED(path.PopFirst(), "");
-  ASSERT_DEATH_IF_SUPPORTED(path.PopFirst(2), "");
-  ASSERT_DEATH_IF_SUPPORTED(path.PopLast(), "");
+  ASSERT_ANY_THROW(path.first_segment());
+  ASSERT_ANY_THROW(path.last_segment());
+  ASSERT_ANY_THROW(path[0]);
+  ASSERT_ANY_THROW(path[1]);
+  ASSERT_ANY_THROW(path.PopFirst());
+  ASSERT_ANY_THROW(path.PopFirst(2));
+  ASSERT_ANY_THROW(path.PopLast());
 }
 
 TEST(FieldPath, Parsing) {
@@ -201,7 +201,7 @@ TEST(FieldPath, Parsing) {
 
   const auto path_with_dot = FieldPath::FromServerFormat(R"(foo\.bar)");
   EXPECT_EQ(path_with_dot.CanonicalString(), "`foo.bar`");
-  EXPECT_EQ(path_with_dot.size(), 1);
+  EXPECT_EQ(path_with_dot.size(), 1u);
 }
 
 // This is a special case in C++: std::string may contain embedded nulls. To
@@ -213,22 +213,22 @@ TEST(FieldPath, ParseEmbeddedNull) {
   str += ".bar";
 
   const auto path = FieldPath::FromServerFormat(str);
-  EXPECT_EQ(path.size(), 1);
+  EXPECT_EQ(path.size(), 1u);
   EXPECT_EQ(path.CanonicalString(), "foo");
 }
 
 TEST(FieldPath, ParseFailures) {
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat(""), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("."), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat(".."), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("foo."), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat(".bar"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("foo..bar"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat(R"(foo\)"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat(R"(foo.\)"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("foo`"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("foo```"), "");
-  ASSERT_DEATH_IF_SUPPORTED(FieldPath::FromServerFormat("`foo"), "");
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat(""));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("."));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat(".."));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("foo."));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat(".bar"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("foo..bar"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat(R"(foo\)"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat(R"(foo.\)"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("foo`"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("foo```"));
+  ASSERT_ANY_THROW(FieldPath::FromServerFormat("`foo"));
 }
 
 TEST(FieldPath, CanonicalStringOfSubstring) {

Firestore/core/test/firebase/firestore/model/resource_path_test.cc

@@ -29,18 +29,18 @@ namespace model {
 TEST(ResourcePath, Constructor) {
   const ResourcePath empty_path;
   EXPECT_TRUE(empty_path.empty());
-  EXPECT_EQ(0, empty_path.size());
+  EXPECT_EQ(0u, empty_path.size());
   EXPECT_TRUE(empty_path.begin() == empty_path.end());
 
   const ResourcePath path_from_list{{"rooms", "Eros", "messages"}};
   EXPECT_FALSE(path_from_list.empty());
-  EXPECT_EQ(3, path_from_list.size());
+  EXPECT_EQ(3u, path_from_list.size());
   EXPECT_TRUE(path_from_list.begin() + 3 == path_from_list.end());
 
   std::vector<std::string> segments{"rooms", "Eros", "messages"};
   const ResourcePath path_from_segments{segments.begin(), segments.end()};
   EXPECT_FALSE(path_from_segments.empty());
-  EXPECT_EQ(3, path_from_segments.size());
+  EXPECT_EQ(3u, path_from_segments.size());
   EXPECT_TRUE(path_from_segments.begin() + 3 == path_from_segments.end());
 
   ResourcePath copied = path_from_list;
@@ -96,8 +96,8 @@ TEST(ResourcePath, Parsing) {
 }
 
 TEST(ResourcePath, ParseFailures) {
-  ASSERT_DEATH_IF_SUPPORTED(ResourcePath::Parse("//"), "");
-  ASSERT_DEATH_IF_SUPPORTED(ResourcePath::Parse("foo//bar"), "");
+  ASSERT_ANY_THROW(ResourcePath::Parse("//"));
+  ASSERT_ANY_THROW(ResourcePath::Parse("foo//bar"));
 }
 
 }  // namespace model

cmake/FindNanopb.cmake

@@ -10,7 +10,7 @@ find_path(
 find_library(
   NANOPB_LIBRARY
   NAMES protobuf-nanopb protobuf-nanopbd
-  HINTS ${BINARY_DIR}/src/nanopb-build
+  HINTS ${BINARY_DIR}/src/nanopb
 )
 
 find_package_handle_standard_args(

cmake/external/nanopb.cmake

@@ -55,9 +55,14 @@ ExternalProject_Add(
   # nanopb relies on $PATH for the location of protoc. cmake makes it difficult
   # to adjust the path, so we'll just patch the build files with the exact
   # location of protoc.
+  #
+  # NB: cmake sometimes runs the patch command multiple times in the same src
+  # dir, so we need to make sure this is idempotent. (eg 'make && make clean &&
+  # make')
   PATCH_COMMAND
-    perl -i -pe s,protoc,${NANOPB_PROTOC_BIN},g
-        ./CMakeLists.txt ./generator/proto/Makefile
+    grep ${NANOPB_PROTOC_BIN} ./generator/proto/Makefile
+      || perl -i -pe s,protoc,${NANOPB_PROTOC_BIN},g
+           ./CMakeLists.txt ./generator/proto/Makefile
 
   UPDATE_COMMAND ""
   INSTALL_COMMAND ""