-
Notifications
You must be signed in to change notification settings - Fork 592
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
Preliminary C++ proxy refactoring #1754
Conversation
@@ -278,10 +272,14 @@ class ICE_API ObjectPrx : public ::std::enable_shared_from_this<ObjectPrx> | |||
{ | |||
public: | |||
|
|||
ObjectPrx(const ObjectPrx& other) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement for _copyFrom .
@@ -1163,7 +1159,7 @@ class Proxy : public virtual Bases... | |||
*/ | |||
::std::shared_ptr<Prx> ice_context(const ::Ice::Context& context) const | |||
{ | |||
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_context(context)); | |||
return ::std::make_shared<Prx>(*ObjectPrx::ice_context(context)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, ObjectPrx::ice_context and friends created a clone of the most derived classes (with virtual newInstance). Now, they just create a new ObjectPrx and we create here the most derived class. It's a bit wasteful but:
a) it doesn't matter since these factory functions aren't called very often
b) when we get rid of shared_ptr for proxies, we can improve all this code
InterfaceList bases = p->bases(); | ||
|
||
H << sp; | ||
writeDocSummary(H, p); | ||
H << nl << "class " << _dllClassExport << p->name() << "Prx : public virtual " | ||
H << nl << "class " << _dllClassExport << p->name() << "Prx : public " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This virtual was not necessary. There is no way derive twice from this base helper class with the same template args.
friend class ::IceInternal::ProxyFactory; | ||
|
||
::IceInternal::ReferencePtr _reference; | ||
const ::IceInternal::ReferencePtr _reference; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectPrx holding this const reference is the pimpl technique:
https://en.cppreference.com/w/cpp/language/pimpl
Note:
- it's pretty C++ specific so it's odd to use this technique in other languages
- the "pointer to implementation" is typically a unique_ptr, not a shared_ptr. We should consider switching to unique_ptr when we refactor IceInternal::Reference
This preliminary Proxy refactoring makes ObjectPrx and derived classes more standard classes.
ObjectPrx's reference data member is now const and initialized by ObjectPrx's constructors.