Continue making Ice::Value the same for the Original and New mappings #1666
Conversation
Remove deprecated ObjectFactory
| @@ -70,7 +21,34 @@ namespace Ice | |||
| * @param type The value type. | |||
| * @return The value created for the given type, or nil if the factory is unable to create the value. | |||
| */ | |||
| using ValueFactory = ::std::function<::std::shared_ptr<Value>(const ::std::string& type)>; | |||
| using ValueFactoryFunc = ::std::function<::std::shared_ptr<Value>(const ::std::string& type)>; | |||
There was a problem hiding this comment.
I reworked ValueFactory and ValueFactoryManager to be a hybrid of the Old and New mappings:
- you can add and find a ValueFactoryFunc (= new mapping API)
- you can also add a ValueFactoryPtr = shared_ptr, where ValueFactory is an abstract base class with a pure virtual create function.
- find only returns the ValueFactoryFunc
| virtual std::shared_ptr<Value> create(const std::string& type) = 0; | ||
| }; | ||
|
|
||
| using ValueFactoryPtr = std::shared_ptr<ValueFactory>; |
There was a problem hiding this comment.
The ultimate goal is for all Ptr in the public API to be shared_ptr all the time, with a few exceptions:
- generated mapped classes (Ptr = SharedPtr or shared_ptr depending on the mapping)
- generated mapped servants
- ValuePtr and ObjectPtr
Since I was updating ValueFactory and ValueFactoryManager, I switched them to the new API.
| ValueFactoryManager() = default; | ||
| ValueFactoryManager(const ValueFactoryManager&) = default; | ||
| ValueFactoryManager& operator=(const ValueFactoryManager&) = default; | ||
| virtual void add(ValueFactoryFunc factory, const ::std::string& id) = 0; |
There was a problem hiding this comment.
That's the only API we use in cpp/.
The ValueFactoryPtr API is used by other language mappings based on C++ like python.
| @@ -16,19 +16,14 @@ struct SliceInfo; | |||
| class SlicedData; | |||
| class UnknownSlicedValue; | |||
|
|
|||
| #ifdef ICE_CPP11_MAPPING | |||
| /// \cond INTERNAL | |||
| using SliceInfoPtr = ::std::shared_ptr<SliceInfo>; | |||
There was a problem hiding this comment.
SliceInfoPtr and SlicedDataPtr are now shared_ptr all the time.
| @@ -66,14 +64,24 @@ class ICE_API Value | |||
| * Returns a shallow copy of the object. | |||
| * @return The cloned value. | |||
| */ | |||
| std::shared_ptr<Value> ice_clone() const; | |||
| #if defined ICE_CPP11_MAPPING | |||
| inline std::shared_ptr<Value> ice_clone() const | |||
There was a problem hiding this comment.
inline ice_clone is one of the very few mapping-specific functions.
| << getUnqualified("::Ice::ValuePtr&", scope) << ");"; | ||
| H << nl << "/// \\endcond"; | ||
| bool | ||
| Slice::Gen::DeclVisitor::visitClassDefStart(const ClassDefPtr& p) |
There was a problem hiding this comment.
Most of the changes in this file are copying the Cpp11 visitors for classes to the Cpp98 with minimal changes.
At a later point--when the implementation of both mappings is closer--we should clean up this code and use the same visitors for both mappings.
| } | ||
| -(void) dealloc | ||
| { | ||
| [valueFactoryManager_ release]; | ||
| [objectFactories_ release]; |
There was a problem hiding this comment.
I believe this was a left over of the object factories removed in an earlier PR
| #include <Ice/SlicedData.h> | ||
|
|
||
| @interface ICESlicedData : NSObject<ICESlicedData> | ||
| { | ||
| @private | ||
| Ice::SlicedData* slicedData_; | ||
| Ice::SlicedDataPtr slicedData_; |
There was a problem hiding this comment.
My understanding here:
we can use a C++ data member with a destructor because the type (shared_ptr) has no virtual function. And the cleanup of ICESlicedData will run the C++ destructors. Would be good to double-check with a small test.
In the previous version, we were using a pointer to the Ptr and made calls to __incRef and __decRef.
| void | ||
| clear(const BPtr& b) | ||
| { | ||
| // No GC with the C++11 mapping | ||
| if(dynamic_pointer_cast<B>(b->theA)) | ||
| if(ICE_DYNAMIC_CAST(B, b->theA)) |
There was a problem hiding this comment.
Macros like ICE_DYNAMIC_CAST are and should remain useful for the tests.
| @@ -647,25 +647,20 @@ class PNodeI : public virtual PNode | |||
|
|
|||
| int PNodeI::counter = 0; | |||
|
|
|||
| #ifndef ICE_CPP11_MAPPING | |||
| // TODO: this class is not used anywhere | |||
There was a problem hiding this comment.
It's not immediately clear from which test it comes. Maybe a GC test?
| void | ||
| IceInternal::ValueFactoryManagerI::add(const ValueFactoryPtr& factory, const string& id) | ||
| { | ||
| IceUtil::Mutex::Lock sync(*this); |
There was a problem hiding this comment.
We should review this code in a separate PR, the "_factoryFuncMapHint` optimization doesn't seem worth it, because the insertion order is not predictable. Adding value factories is also not a critical code path.
This PR updates Ice::Value to be almost identical with the Original and New mappings.
In particular, InputStream and OutputStream always encodes and decodes
shared_ptr<Ice::Value>. More generally, the Ice core longer uses "Ice::ValuePtr" anywhere.