Use C++'s smart pointers std::unique_ptr and std::shared_ptr instead of our own. #1365
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
In this release, the "src" directory is removed from boringssl, so there are some CMake and patch changes necessary for compatibility.
Revert Firestore.
jonsimantov added tests-requested: quick 
github-actions bot added tests: in-progress 
Integration test with FLAKINESS (succeeded after retry)Requested by @a-maurice on commit 6a59781
Add flaky tests to go/fpl-cpp-flake-tracker |
firebase-workflow-trigger bot removed the tests: in-progress jonsimantov added the tests-requested: quick github-actions bot added tests: in-progress jonsimantov added the tests-requested: quick github-actions bot removed tests-requested: quick jonsimantov added the tests-requested: quick github-actions bot removed tests-requested: quick 
dconeybe reviewed Jun 30, 2023
github-actions bot added the tests: succeeded firebase-workflow-trigger bot removed the tests: in-progress 
| This is ready for review, all integration and unit tests should now be working. |
a-maurice reviewed Jul 1, 2023
| | ||
| // Safe reference to this. Set in constructor and cleared in destructor | ||
| // Should be safe to be copied in any thread because the SharedPtr never | ||
| // Should be safe to be copied in any thread because the std::shared_ptr never |
There was a problem hiding this comment.
Any idea what shared_ptr this is referring to?
a-maurice approved these changes Jul 5, 2023
github-actions bot added tests: in-progress firebase-workflow-trigger bot removed the tests: in-progress tom-andersen pushed a commit that referenced this pull request Jul 17, 2023
…of our own. (#1365) * Remove headers from CMake file. * Find-and-replace change. * Format code. * Find and replace for std::atomic. * Format code. * Update to Dec 2022 release of boringssl. In this release, the "src" directory is removed from boringssl, so there are some CMake and patch changes necessary for compatibility. * Re-add patch file. * Switch to std::make_unique. * Fix make_shared, std::move, and class initialization. * WIP. * Fix more move syntax. * Fix more std::move issues. Revert Firestore. * Remove debug output * Fix Android Firestore test, and Storage assignment. * Make the callback safer via shared_ptr. * Fix lint errors. * Format code. * More lint fixes. * More lint. * Additional lint warning fixes. * Fix Firestore test. * Fix more lints. * Lint lint lint. * Change RequestDataPtr to a unique_ptr since STLPort is no longer used. * Remove Move() and Forward(), replacing with std::move() and std::foward<>(). * Format code and lint fixes. * Change firebase::Move explicitly to std::move. * Remove lint warnings for headers. * More lint warnings on includes. * Format code. * Return back to shared pointer. * Add note about NewCallback and link to TODO. * Fix lint error. * Fix a lingering firebase::Move. * Fix optional_test to mock Move correctly again. * Revert prereqs script, python3 change will be made elsewhere. * Lint. * Remove references to atomic_test. * Fix Move -> std::move once more * Fix a seg fault race condition on Linux by using a unique_ptr. This code always wanted to use a unique_ptr, but was previously using a SharedPtr because of STLPort. However, even without STLPort, there is still an issue switching it to a unique_ptr, which is that std::priority_queue is misdesigned and does not return a mutable reference in C++14. Because of this, if we want to std::move from a priority queue, we need to do a const cast. * Empty commit. * Fix test error. * Fix test compilation error. * Fix additional test build error. * Vector initializer lists use copy semantics. Switch to push_back since these are just tests. * Fix Forward --> std::forward from the merged Firestore change. * Add callback tests for shared_ptr wrapping a unique_ptr.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Description
Switch from firebase::UniquePtr (and flatbuffers::UniquePtr) to std::unique_ptr, and from firebase::SharedPtr to std::shared_ptr. Also updates the use of MakeUnique / MakeShared to their std::make_unique and std::make_shared equivalents.
This also removes app/meta/move.h, which provided Move() and Forward<>(); it will now use std::move and std::forward, in the header, instead. Firestore uses these a lot.
The purpose of this PR is to clean up our code base, using modern C++ smart pointers instead of our own equivalents, to make it easier to maintain in the future.
This PR also replaces rare usage of firebase::compat::Atomic with std::atomic (mainly in a Firestore test, since the main usage used to be inside our own SharedPtr class that is now deleted).
Important: buried in this change is a modification to database/src/desktop/query_desktop.cc, since the Callback mechanism does not work with move-only types (such as std::unique_ptr). This change wraps the unique_ptr inside a shared_ptr, thus making it copyable. Inside the callback, it releases the raw pointer from the unique_ptr and wraps it in a new unique_ptr. This ensures that there is never a time in which the pointer memory is not managed somewhere (it's either owned by the shared_ptr's unique_ptr prior to the callback, or by the newly created unique_ptr after the callback).
Testing
Running integration tests in this PR, as well as the unit tests.
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.