Port FSTListenerRegistration to C++ #2619
Conversation
... and use it
These are implicitly deleted because DocumentSet is a member and isn't default constructible.
|
|
||
| class ListenerRegistration { | ||
| public: | ||
| ListenerRegistration() = default; |
There was a problem hiding this comment.
Is this necessary / desirable? Seems like you should always use the params constructor...
There was a problem hiding this comment.
This is required in order to make this work:
The idea is that the shared_ptr is constructed, pointing to a default instance. That instance is later filled in once the listen starts.
As a practical matter std::shared_ptr (at least in libc++) can only be used on types that are default constructible. #2620 changes this to a std::promise where I don't think that requirement exists, and I may be able to remove this. I've implemented it in this way here in order to contain the blast radius of the changes.
There was a problem hiding this comment.
Ah, I see! Okay, that seems cool, though I would be a fan of removing it if indeed #2620 removes the requirement (and don't forget to remove the 2 comments you added about it :-)).
| internal_listener_(std::move(internal_listener)) { | ||
| } | ||
|
|
||
| // Move-only to prevent copies from proliferating. |
There was a problem hiding this comment.
Does this impose additional restrictions on how people use our API? E.g. As a consumer, would I be forced to use std::move() where if we had a copy constructor, I wouldn't have to?
If yes, what is the actual benefit of "prevent copies from proliferating"?
There was a problem hiding this comment.
That's an interesting question.
I had initially done it this way while thinking about Objective-C clients, wherein the ListenerReference is embedded in an FSTListenerRegistration and a user coping that does so by reference.
Making this type move-only helps keep us honest while propagating this out to end users, and avoids us accidentally copying when we should have moved.
However, you're right: in the public C++ API (even after you look past the PIMPL stuff) we do want users to be able to copy the public listener registration, so we should allow that here. Removed.
wilhuff
force-pushed
the
wilhuff/listener-registration
branch
from
fdaeb47 to
f2b254b
Compare
|
|
||
| class ListenerRegistration { | ||
| public: | ||
| ListenerRegistration() = default; |
There was a problem hiding this comment.
Ah, I see! Okay, that seems cool, though I would be a fan of removing it if indeed #2620 removes the requirement (and don't forget to remove the 2 comments you added about it :-)).
| @@ -37,6 +37,30 @@ namespace firebase { | |||
| namespace firestore { | |||
| namespace api { | |||
|
|
|||
| /** | |||
| * An internal handle that encapsulates a user's ability to request that we | |||
There was a problem hiding this comment.
Hrm... Is this class really "internal"? In Obj-C the class is internal because it implements a public interface that's part of our API. But we don't seem to have an interface here (at least at the moment)? So it seems like this will be part of our public C++ API? And in that case, this (awesome) comment might need a rewrite to be more suitable as customer-facing docs. :-/
On that note, the constructor isn't meant to be part of our public API. Is there a way to hide it or discourage its usage?
There was a problem hiding this comment.
Everything under Firestore/core is internal: it's the core for actually implementing various language bindings, including the Objective-C and public C++ bindings. The firebase::firestore::api namespace is intended to closely resemble the other ports in terms of function, but isn't publicly facing, it's still just a building block for an actual public APIs.
See go/firestore-cpp-integration-plan for more details.
wilhuff
force-pushed
the
wilhuff/query-listener
branch
from
f1f24b9 to
d85bf4e
Compare
* Replace FSTQueryListener with C++ QueryListener * objc::unordered_map * Add a lint check for Objective-C banned words
Unlike most of the other ports in this series, the original Objective-C class must remain, since you can't implement an Objective-C protocol in a C++ class. This is despite the "FST" in the name: the public type is a protocol so we actually need to keep the FSTListenerRegistration as well.
Note the DIFFBASE on #2618.