Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake build fixes #770

Merged
merged 7 commits into from
Feb 9, 2018
Merged

cmake build fixes #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b7552d6
Fix nanopb (in cmake build)
rsgowman Feb 7, 2018
4312101
Add resource_path.{h,cc} to the cmake build
rsgowman Feb 7, 2018
ee5167a
Fix signed/unsigned int comparison warnings
rsgowman Feb 7, 2018
bf6bebe
Ensure FieldValue tag_ is initialized during cp/mv ctor.
rsgowman Feb 7, 2018
0848bf3
Fix tests that throw exceptions.
rsgowman Feb 7, 2018
7a7a6bd
./scripts/lint.sh
rsgowman Feb 7, 2018
06ad487
Move FieldValue::tag_ initializer to be in class.
rsgowman Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Firestore/core/src/firebase/firestore/model/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Firestore/core/src/firebase/firestore/model/base_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions Firestore/core/src/firebase/firestore/model/field_path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Firestore/core/src/firebase/firestore/model/field_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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
Expand Down
4 changes: 2 additions & 2 deletions Firestore/core/src/firebase/firestore/model/field_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class FieldValue {
// position instead, see the doc comment above.
};

FieldValue() : tag_(Type::Null) {
FieldValue() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: this now can be = default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem to work... (Due to the union with nontrivial types.)

eg, this fails to compile:

#include <string>

class Abc {
 public:
  Abc() = default;

  union {
    std::string c;
  };
};
    
int main() {
  Abc abc;
}

}

// Do not inline these ctor/dtor below, which contain call to non-trivial
Expand Down Expand Up @@ -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_;
Expand Down
1 change: 1 addition & 0 deletions Firestore/core/src/firebase/firestore/model/resource_path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 3 additions & 2 deletions Firestore/core/src/firebase/firestore/model/resource_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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_
46 changes: 23 additions & 23 deletions Firestore/core/test/firebase/firestore/model/field_path_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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) {
Expand Down
10 changes: 5 additions & 5 deletions Firestore/core/test/firebase/firestore/model/resource_path_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cmake/FindNanopb.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
9 changes: 7 additions & 2 deletions cmake/external/nanopb.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
Expand Down