fix(firestore): add noexcept specifiers to move, swap, hash operations - #16117
Conversation
b/269108721
Adds `noexcept` specifiers to move, swap, and hash operations throughout
the SDK.
According the C++ Core Guidelines, move, swap, and hash operations
should be `noexcept`.
If move operations are throwable, many std lib containers that use these
objects will fall back to using copy semantics, degrading performance.
I used grep to attempt to locate all instances of these operations
that did not have the `noexcept` specifiers.
Command:
```
rg --pcre2 -n -g '*.{h,cc}' '\b(?:friend\s+)?(?:void|size_t|std::size_t|auto)\s+(?:swap|hash|operator\s*\(\))\s*\(' | rg -v 'noexcept'
```
Results:
Deleted constructors. No action required.
- util/statusor_internals.h:248: TraitsBase(TraitsBase&&) = delete;
- util/statusor_internals.h:250: TraitsBase& operator=(TraitsBase&&) = delete;
- util/ordered_code.h:155: OrderedCode(OrderedCode&&) = delete;
- util/ordered_code.h:156: OrderedCode& operator=(OrderedCode&&) = delete;
- remote/datastore.h:154: Datastore(Datastore&& other) = delete;
- remote/datastore.h:156: Datastore& operator=(Datastore&& other) = delete;
- util/testing_hooks.h:155: TestingHooks(TestingHooks&&) = delete;
- util/testing_hooks.h:157: TestingHooks& operator=(TestingHooks&&) = delete;
- local/memory_document_overlay_cache.h:39: MemoryDocumentOverlayCache(MemoryDocumentOverlayCache&&) = delete;
- local/memory_document_overlay_cache.h:40: MemoryDocumentOverlayCache& operator=(MemoryDocumentOverlayCache&&) = delete;
- local/leveldb_document_overlay_cache.h:51: LevelDbDocumentOverlayCache(LevelDbDocumentOverlayCache&&) = delete;
- local/leveldb_document_overlay_cache.h:52: LevelDbDocumentOverlayCache& operator=(LevelDbDocumentOverlayCache&&) = delete;
Default constructors. Are implicitly noexcept. Explicitly make noexcept.
[x] remote/bloom_filter.h:35: BloomFilter(BloomFilter&&) = default;
[x] remote/bloom_filter.h:37: BloomFilter& operator=(BloomFilter&&) = default;
[x] util/statusor_internals.h:230: TraitsBase(TraitsBase&&) = default;
[x] util/statusor_internals.h:232: TraitsBase& operator=(TraitsBase&&) = default;
[x] util/statusor_internals.h:239: TraitsBase(TraitsBase&&) = default;
[x] util/statusor_internals.h:241: TraitsBase& operator=(TraitsBase&&) = default;
[x] util/random_access_queue.h:59: RandomAccessQueue(RandomAccessQueue&& other) = default;
[x] util/random_access_queue.h:68: RandomAccessQueue& operator=(RandomAccessQueue&& other) = default;
[x] api/settings.h:52: Settings(Settings&& other) = default;
[x] api/settings.h:55: Settings& operator=(Settings&& other) = default;
Utils copied, this is not code written by us. No action required.
[ ] util/status.h:140: void operator()(const State* ptr) const;
[ ] util/statusor.h:166: StatusOr& operator=(Status&& status);
[ ] util/statusor.h:243:StatusOr<T>& StatusOr<T>::operator=(Status&& status) {
[ ] util/statusor.h:116: StatusOr(StatusOr&&) = default;
[ ] util/statusor.h:117: StatusOr& operator=(StatusOr&&) = default;
[x] model/document_key.h:113: size_t operator()(const DocumentKey& key) const;
[x] model/overlay.h:79: std::size_t operator()(const Overlay&) const;
- could be an issue if Mutation is not nothrow hashable. It hashes type, key, mutation, precondition. I think these should all be nothrow hashable.
[ ] model/field_path.cc:84: void operator()(T* out, const std::string& segment) {
- i don't think this can be made noexcept, since it's templated. T-> could throw an exception.
[x] core/query.h:332: size_t operator()(const firebase::firestore::core::Query& query) const {
- Query::Has() -> absl::StrCat() -> std::badalloc in OOM. Already fatal. std::terminate ok. use noexcept.
[x] core/pipeline_util.h:204: size_t operator()(
- Query::Has() -> absl::StrCat() -> std::badalloc in OOM. Already fatal. std::terminate ok. use noexcept.
[x] core/pipeline_util.h:212: size_t operator()(
- Query::Has() -> absl::StrCat() -> std::badalloc in OOM. Already fatal. std::terminate ok. use noexcept.
[x] core/target.h:245: size_t operator()(const firebase::firestore::core::Target& target) const {
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
cherylEnkidu
left a comment
There was a problem hiding this comment.
Thank u for this improvement!
b/269108721
Adds
noexceptspecifiers to move, swap, and hash operations throughout the SDK.According the C++ Core Guidelines, move, swap, and hash operations should be
noexcept.If move operations are throwable, many std lib containers that use these objects will fall back to using copy semantics, degrading performance.
I used grep to attempt to locate all instances of these operations that did not have the
noexceptspecifiers.Command:
Notes:
noexceptanyway to be more explicit.absl::StrCatwhich can throwstd::bad_allocif memory can't be allocated (OOM). If we label these hash operations asnoexcept, and then astd::bad_allocis thrown, thenstd::terminatewill be called [1]. This is ok, since OOM conditions are already fatal and cause apps to crash.[1] Whenever an exception is thrown and the search for a handler encounters the outermost block of a non-throwing function, the function std::terminate is called https://en.cppreference.com/cpp/language/noexcept_spec