Skip to content

Fix "Undefined symbols" linker error when DocumentChange::npos was used #474

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

Merged
merged 6 commits into from
Jun 21, 2021
Merged

Fix "Undefined symbols" linker error when DocumentChange::npos was used #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77d3475
Fix "Undefined symbols" linker error when DocumentChange::npos was used.
dconeybe Jun 16, 2021
ee4fa77
Add change log entry
dconeybe Jun 16, 2021
3be0e69
Fix version number in change log entry (8.0.1 -> 8.1.0)
dconeybe Jun 16, 2021
544ae52
Merge remote-tracking branch 'origin/main' at 3803777c066d675909241ca…
dconeybe Jun 17, 2021
005ae9b
Merge 'origin/main' at 287f764682e9cba15cedb8da4492ee495fa09684
dconeybe Jun 21, 2021
1e1c41d
Add backticks around DocumentChange::npos.
dconeybe Jun 21, 2021
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
8 changes: 8 additions & 0 deletions firestore/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,14 @@ TEST_F(FirebaseFirestoreBasicTest, TestQuery) {
"int", firebase::firestore::FieldValue::Integer(101))))));
}

TEST_F(FirebaseFirestoreBasicTest, TestDocumentChangeNpos) {
// This test may seem pointless, but it exists to avoid the long-standing
// latent bug that `npos` was not defined on non-Android platforms and
// would therefore fail to link if used.
EXPECT_EQ(firebase::firestore::DocumentChange::npos,
static_cast(-1));
}

TEST_F(FirebaseFirestoreBasicTest,
TestInvalidatingReferencesWhenDeletingFirestore) {
delete firestore_;
Expand Down
8 changes: 8 additions & 0 deletions firestore/integration_test_internal/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,14 @@ TEST_F(FirebaseFirestoreBasicTest, TestQuery) {
"int", firebase::firestore::FieldValue::Integer(101))))));
}

TEST_F(FirebaseFirestoreBasicTest, TestDocumentChangeNpos) {
// This test may seem pointless, but it exists to avoid the long-standing
// latent bug that `npos` was not defined on non-Android platforms and
// would therefore fail to link if used.
EXPECT_EQ(firebase::firestore::DocumentChange::npos,
static_cast(-1));
}

TEST_F(FirebaseFirestoreBasicTest,
TestInvalidatingReferencesWhenDeletingFirestore) {
delete firestore_;
Expand Down
2 changes: 2 additions & 0 deletions firestore/src/common/document_change.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ using Type = DocumentChange::Type;
// Older NDK (r16b) fails to define this properly. Fix this when support for
// the older NDK is removed.
const std::size_t DocumentChange::npos = static_cast(-1);
#else
constexpr std::size_t DocumentChange::npos;
#endif // defined(ANDROID)

DocumentChange::DocumentChange() {}
Expand Down
5 changes: 5 additions & 0 deletions release_build_files/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,11 @@ code.

## Release Notes

### 8.1.0
- Changes
- Firestore: Fixed a linker error when `DocumentChange::npos` was used.
([#474](https://github.com/firebase/firebase-cpp-sdk/pull/474)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: I think the changelog usually links to issues, not PRs, so this link could probably be omited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there is a mixture of issues and PRs linked to in previous change log entries. I see your point but I personally feel like adding a link to additional context is useful for future readers. If this issue had been discovered externally then there would be an issue number; however, since it was discovered by me, a developer on the Firestore team, no issue was created. IMO, providing this link adds value at no cost. If you feel strongly I can remove it, but I like having it here.


### 8.0.0
- Changes
- Analytics: Removed `SetCurrentScreen()` following its removal from iOS SDK
Expand Down