Skip to content

Add a nanopb string #1839

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.

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

Merged
merged 7 commits into from
Nov 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions Firestore/Example/Firestore.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@
7346E61D20325C6900FD6CEF /* FSTDispatchQueueTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7346E61C20325C6900FD6CEF /* FSTDispatchQueueTests.mm */; };
73866AA12082B0A5009BB4FF /* FIRArrayTransformTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 73866A9F2082B069009BB4FF /* FIRArrayTransformTests.mm */; };
73FE5066020EF9B2892C86BF /* hard_assert_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 444B7AB3F5A2929070CB1363 /* hard_assert_test.cc */; };
84DBE646DCB49305879D3500 /* nanopb_string_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 353EEE078EF3F39A9B7279F6 /* nanopb_string_test.cc */; };
873B8AEB1B1F5CCA007FD442 /* Main.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 873B8AEA1B1F5CCA007FD442 /* Main.storyboard */; };
8C82D4D3F9AB63E79CC52DC8 /* Pods_Firestore_IntegrationTests_iOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = ECEBABC7E7B693BE808A1052 /* Pods_Firestore_IntegrationTests_iOS.framework */; };
AB356EF7200EA5EB0089B766 /* field_value_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB356EF6200EA5EB0089B766 /* field_value_test.cc */; };
Expand Down Expand Up @@ -308,6 +309,7 @@
2A0CF41BA5AED6049B0BEB2C /* type_traits_apple_test.mm */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.objcpp; path = type_traits_apple_test.mm; sourceTree = "<group>"; };
2B50B3A0DF77100EEE887891 /* Pods_Firestore_Tests_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Tests_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; };
332485C4DCC6BA0DBB5E31B7 /* leveldb_util_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_util_test.cc; sourceTree = "<group>"; };
353EEE078EF3F39A9B7279F6 /* nanopb_string_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; name = nanopb_string_test.cc; path = nanopb/nanopb_string_test.cc; sourceTree = "<group>"; };
358C3B5FE573B1D60A4F7592 /* strerror_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = strerror_test.cc; sourceTree = "<group>"; };
3B843E4A1F3930A400548890 /* remote_store_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = remote_store_spec_test.json; sourceTree = "<group>"; };
3C81DE3772628FE297055662 /* Pods-Firestore_Example_iOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_iOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_iOS/Pods-Firestore_Example_iOS.debug.xcconfig"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -729,6 +731,7 @@
54EB764B202277970088B8F3 /* immutable */,
54995F70205B6E1A004EFFA0 /* local */,
AB356EF5200E9D1A0089B766 /* model */,
5C332D7293E6114E491D3662 /* nanopb */,
546854A720A3681B004BDBD5 /* remote */,
5467FB05203E652F009C9584 /* testutil */,
54740A561FC913EB00713A1A /* util */,
Expand Down Expand Up @@ -781,6 +784,14 @@
path = immutable;
sourceTree = "<group>";
};
5C332D7293E6114E491D3662 /* nanopb */ = {
isa = PBXGroup;
children = (
353EEE078EF3F39A9B7279F6 /* nanopb_string_test.cc */,
);
name = nanopb;
sourceTree = "<group>";
};
5CAE131A20FFFED600BE9A4A /* Benchmarks */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -1939,6 +1950,7 @@
AB6B908620322E6D00CC290A /* maybe_document_test.cc in Sources */,
618BBEA820B89AAC00B5BCE7 /* mutation.pb.cc in Sources */,
32F022CB75AEE48CDDAF2982 /* mutation_test.cc in Sources */,
84DBE646DCB49305879D3500 /* nanopb_string_test.cc in Sources */,
AB6B908820322E8800CC290A /* no_document_test.cc in Sources */,
AB380D04201BC6E400D97691 /* ordered_code_test.cc in Sources */,
5A080105CCBFDB6BF3F3772D /* path_test.cc in Sources */,
Expand Down
1 change: 1 addition & 0 deletions Firestore/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,6 @@ add_subdirectory(test/firebase/firestore/core)
add_subdirectory(test/firebase/firestore/immutable)
add_subdirectory(test/firebase/firestore/local)
add_subdirectory(test/firebase/firestore/model)
add_subdirectory(test/firebase/firestore/nanopb)
add_subdirectory(test/firebase/firestore/remote)
add_subdirectory(test/firebase/firestore/util)
4 changes: 3 additions & 1 deletion Firestore/core/src/firebase/firestore/nanopb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
cc_library(
firebase_firestore_nanopb
SOURCES
tag.h
nanopb_string.cc
nanopb_string.h
reader.h
reader.cc
tag.h
writer.h
writer.cc
DEPENDS
Expand Down
66 changes: 66 additions & 0 deletions Firestore/core/src/firebase/firestore/nanopb/nanopb_string.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_string.h"

#include <cstdlib>
#include <utility>

namespace firebase {
namespace firestore {
namespace nanopb {

/* static */ pb_bytes_array_t* String::MakeBytesArray(absl::string_view value) {
auto size = static_cast<pb_size_t>(value.size());

// Allocate one extra byte for the null terminator that's not necessarily
// there in a string_view. As long as we're making a copy, might as well
// make a copy that won't overrun when used as a regular C string. This is
// essentially just to make debugging easier--actual user data can have
// embedded nulls so we shouldn't be using this as a C string under normal
// circumstances.
auto result = static_cast<pb_bytes_array_t*>(
malloc(PB_BYTES_ARRAY_T_ALLOCSIZE(size) + 1));
result->size = size;
memcpy(result->bytes, value.data(), size);
result->bytes[size] = '\0';

return result;
}

pb_bytes_array_t* String::release() {
pb_bytes_array_t* result = bytes_;
bytes_ = nullptr;
return result;
}

void swap(String& lhs, String& rhs) noexcept {
using std::swap;
swap(lhs.bytes_, rhs.bytes_);
}

/* static */ String String::Wrap(pb_bytes_array_t* bytes) {
return String{bytes};
}

/* static */ absl::string_view String::ToStringView(pb_bytes_array_t* bytes) {
const char* str = reinterpret_cast<const char*>(bytes->bytes);
return absl::string_view{str, bytes->size};
}

} // namespace nanopb
} // namespace firestore
} // namespace firebase
153 changes: 153 additions & 0 deletions Firestore/core/src/firebase/firestore/nanopb/nanopb_string.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_NANOPB_NANOPB_STRING_H_
Copy link
Member

Choose a reason for hiding this comment

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

You'll eventually want some unit tests for this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_NANOPB_NANOPB_STRING_H_

#include <pb.h>

#include <string>
#include <utility>

#include "Firestore/core/src/firebase/firestore/util/comparison.h"
#include "absl/strings/string_view.h"

namespace firebase {
namespace firestore {
namespace nanopb {

/**
* A string-like object backed by a nanopb byte array.
*/
class String : public util::Comparable<String> {
Copy link
Member

Choose a reason for hiding this comment

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

docstrings might be nice. Specifically, something mentioning that the ctor copies it's arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. As submitted I was trying to figure out if it was worth investing in such things.

public:
/**
* Creates a new, null-terminated byte array that's a copy of the given string
* value.
*/
static pb_bytes_array_t* MakeBytesArray(absl::string_view value);

String() {
}

/**
* Creates a new String whose backing byte array is a copy of the of the
* given C string.
*/
explicit String(const char* value) : bytes_{MakeBytesArray(value)} {
}

/**
* Creates a new String whose backing byte array is a copy of the of the
* given string.
*/
explicit String(const std::string& value) : bytes_{MakeBytesArray(value)} {
}

/**
* Creates a new String whose backing byte array is a copy of the of the
* given string_view.
*/
explicit String(absl::string_view value) : bytes_{MakeBytesArray(value)} {
}

String(const String& other)
: bytes_{MakeBytesArray(absl::string_view{other})} {
}

String(String&& other) noexcept : String{} {
swap(*this, other);
}

~String() {
delete bytes_;
}

String& operator=(String other) {
Copy link
Member

Choose a reason for hiding this comment

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

Performance nit: Rather than this function, you could implement:

String& operator=(const STring& other) { ... }
String& operator=(STring&& other) { ... }

This would allow you to eliminate the copy in the move case.

If you want to do this, but don't care to now, you could just defer it with a TODO.

swap(*this, other);
Copy link
Member

Choose a reason for hiding this comment

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

Spelling swap as std::swap would allow you to eliminate the using statement. Also below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the using statement here because it's clearly the case that we want to invoke the friend function below.

However, in the other case using std::swap is preferred because it enables ADL. Admittedly it's not that interesting in this case but it matters in other circumstances and should be our preferred way to invoke std::swap.

return *this;
}

/**
* Creates a new String that takes ownership of the given byte array.
*/
static String Wrap(pb_bytes_array_t* bytes);

bool empty() const {
return !bytes_ || bytes_->size == 0;
}

/**
* Returns a pointer to the character data backing this String. The return
* value is `nullptr` if the backing bytes are themselves null.
*/
const char* data() const {
return bytes_ ? reinterpret_cast<const char*>(bytes_->bytes) : nullptr;
}

/** Returns a const view of the underlying byte array. */
const pb_bytes_array_t* get() const {
Copy link
Member

Choose a reason for hiding this comment

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

An alternative: string_view calls it's equivalent method data().

return bytes_;
}

/**
* Returns the current byte array and assigns the backing byte array to
* nullptr, releasing the ownership of the array contents to the caller.
*/
pb_bytes_array_t* release();

/**
* Converts this String to an absl::string_view (without changing ownership).
*/
explicit operator absl::string_view() const {
return ToStringView(bytes_);
}

/**
* Swaps the contents of the given Strings.
*/
friend void swap(String& lhs, String& rhs) noexcept;

friend bool operator==(const String& lhs, const String& rhs) {
return absl::string_view{lhs} == absl::string_view{rhs};
}
friend bool operator<(const String& lhs, const String& rhs) {
return absl::string_view{lhs} < absl::string_view{rhs};
}

friend bool operator==(const String& lhs, absl::string_view rhs) {
absl::string_view lhs_view{lhs};
return lhs_view == rhs;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with the above operators, you could implement this as:
return absl::string_view{lhs} == rhs;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For whatever reason that trips up the linter. We can either do this or something like:

return absl::string_view{lhs} == rhs;  // NOLINT(whitespace/braces)

Copy link
Member

Choose a reason for hiding this comment

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

Well that's strange. Either is fine with me; there's no point fighting the linter.

}

friend bool operator!=(const String& lhs, absl::string_view rhs) {
return !(lhs == rhs);
}

private:
explicit String(pb_bytes_array_t* bytes) : bytes_{bytes} {
}

static absl::string_view ToStringView(pb_bytes_array_t* bytes);

pb_bytes_array_t* bytes_ = nullptr;
};

} // namespace nanopb
} // namespace firestore
} // namespace firebase

#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_NANOPB_NANOPB_STRING_H_
Copy link
Member

Choose a reason for hiding this comment

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

You might consider moving some of the implementation to a .cc file. Though leaving it inline here actually seems ok too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

22 changes: 22 additions & 0 deletions Firestore/core/src/firebase/firestore/util/comparison.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,28 @@ bool DoubleBitwiseEquals(double left, double right);
*/
size_t DoubleBitwiseHash(double d);

template <typename T>
class Equatable {
public:
friend bool operator!=(const T& lhs, const T& rhs) {
return !(lhs == rhs);
}
};

template <typename T>
class Comparable : public Equatable<T> {
public:
friend bool operator>(const T& lhs, const T& rhs) {
return rhs < lhs;
}
friend bool operator<=(const T& lhs, const T& rhs) {
return !(rhs < lhs);
}
friend bool operator>=(const T& lhs, const T& rhs) {
return !(lhs < rhs);
}
};

} // namespace util
} // namespace firestore
} // namespace firebase
Expand Down
21 changes: 21 additions & 0 deletions Firestore/core/test/firebase/firestore/nanopb/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

cc_test(
firebase_firestore_nanopb_test
SOURCES
nanopb_string_test.cc
DEPENDS
firebase_firestore_nanopb
)
Loading