SharedPtr - potential pitfall that can be easily fixed

Everything about the development of Urho3D.

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 10 Apr 2016, 14:05

Maybe I'm missing something here, but how does your solution solve the problem in the first post?

Because from what I understand, you basically disagree with everything I proposed. I want to make these changes to fix a problem I encountered.
1) Force explicit use of SharedPtr<T>::Get() instead of relying on implicit conversions
2) Add bool operator() const { return ptr_ != NULL; }

You appear to disagree with 1) because of "code bloat". However, your solution doesn't fix any of the issues in my first post, so I don't know where that leaves us.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by Dave82 » 10 Apr 2016, 15:28

gawag : You are using an Item* but want to manage that resource. There's nothing hinting towards any kind of resource management as it is just a naked pointer. It would be clear if it would be a SharedPtr<Item> and the Item would be passed as a SharedPtr<Item>. We are in the age of smart pointers and having a naked pointer but wanting any kind of smart behavior from it can be considered a bug.


That's exacly what i thought too.

if a function returns a smart pointer you are responsible to pass it as a smart pointer.
This :
Code: Select all
Item* item = Item::create(ITEM_FLOWER);

looks like you intentionally try to make a mistake.If the return type is SharedPtr the use a SharedPtr
Code: Select all
SharedPtr<Item> item = Item::create(ITEM_FLOWER);
Infested : An old school puzzle solving survival horror game :
topic1430.html

Infested Facebook page
https://www.facebook.com/infestedgame
User avatar
Dave82
Active user
Active user
 
Posts: 136
Joined: 11 May 2015, 18:08

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 10 Apr 2016, 15:46

TheComet wrote:Maybe I'm missing something here, but how does your solution solve the problem in the first post?

I'm getting confused as well.

TheComet wrote:Because from what I understand, you basically disagree with everything I proposed. I want to make these changes to fix a problem I encountered.
1) Force explicit use of SharedPtr<T>::Get() instead of relying on implicit conversions

Yes we agree on the implicit conversion being bad. But I saw problems resulting in removing that and tried fixing those.

TheComet wrote:2) Add bool operator() const { return ptr_ != NULL; }

That's also good, std::shared_ptr does that too.

TheComet wrote:You appear to disagree with 1) because of "code bloat". However, your solution doesn't fix any of the issues in my first post, so I don't know where that leaves us.

I don't disagree but it would force to add .Get() to every place where the implicit conversion was used (breaking old code). My solution fixes this at the places where a naked pointer was used "properly" (without any semantic meaning or sharing) without having the issues you mentioned (as it is not used shared).
In Terrain::SetHeightmap the Image* is actually used shared (if I got that right) but that is confusing as a naked pointer doesn't hint to that. So I suggested passing the SharedPtr either per value (extra cost) or per reference in such cases where the object is actually used in a shared way.

Oh I just notized this: Texture2D::SetData(SharedPtr<Image>). That function is actually getting passed a SharedPtr... And that function isn't using the Image shared at all. It's copying the data.
Why is Texture2D::SetData getting passed a SharedPtr but not using the Image in any way shared but Terrain::SetHeightmap is receiving the Image as a naked pointer but using the reference counting feature (using it shared)? Smells fishy. :roll:

Basically I'm suggesting to use a converter class in functions that don't use the ressource shared like Texture2D::SetData as it would allow to pass SharedPtr<Image> in there as well as Image* and that without having an implicit T* conversion in SharedPtr (as we want to get rid of that unsafe implicitness). And to use SharedPtr (potentially as a reference or somehow "disguised" for speed) in functions like Terrain::SetHeightmap that actually use the resource shared.
AnyPtr is also kinda clumsy especially when passing a naked pointer but I don't see a better solution if one doesn't want the implicit cast SharedPptr::T* or function overloads everywhere for every "pointer type".

Oh Dave82 posted while I was writing this. I completely agree with that post. *nonexisting thumbs up smiley*

Edit: Oh isn't a SharedPtr of a RefCounted object not kinda weird and doubled shared? Oh they work in conjunction, that's still odd.

Edit2: Also SharedPtr can't be used with any class: "error: 'class person' has no member named 'AddRef'" "error: 'class person' has no member named 'ReleaseRef'"
Someone not knowing that Urho specialty "But I thought it would be like std::shared_ptr or QSharedPointer or boost::shared_ptr or ... :( "
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 10 Apr 2016, 16:40

Dave82 wrote:This :
Code: Select all
Item* item = Item::create(ITEM_FLOWER);

looks like you intentionally try to make a mistake.If the return type is SharedPtr the use a SharedPtr
Code: Select all
SharedPtr<Item> item = Item::create(ITEM_FLOWER);


You're missing the point. I'm saying it shouldn't be possible for this to compile:
Code: Select all
Item* item = Item::create(ITEM_FLOWER);


Of course you have to use a SharedPtr if the return type is a SharedPtr, but the fact of the matter is, if you happen to forget it is returning a SharedPtr and you use a raw pointer accidentally, the code compiles and runs anyway (and will later crash). This is what I'm trying to fix. Does that make sense?

gawag wrote:My solution fixes this at the places where a naked pointer was used "properly" (without any semantic meaning or sharing) without having the issues you mentioned (as it is not used shared).


No, it still has the issues I mentioned. This code I posted earlier - which uses your AnyPtr - shows exactly how the issue still exists.
Code: Select all
class Item {
public:
    static SharedPtr<Item> Item::create(Item_e item) {
        switch(item) {
            case ITEM_FLOWER: return new FlowerItem;
            /* etc */
        }
        return SharedPtr<Item>();
    }
};

class Player {
    Item* item_;
public:
    void HoldItem(AnyPtr<Item> item) {
        item_ = item;
    }
}


Code: Select all
Player player;
player.HoldItem(Item::create(ITEM_FLOWER));
// Oh oh, player.item_ is now pointing to a destroyed object
// This is exactly what I'm trying to fix, AnyPtr re-introduces the issue


I still fail to see how offloading the implicit conversion to a new wrapper class is any different than just leaving the implicit conversion as-is?
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 10 Apr 2016, 17:11

TheComet wrote:No, it still has the issues I mentioned. This code I posted earlier - which uses your AnyPtr - shows exactly how the issue still exists.
Code: Select all
...
class Player {
    Item* item_;
public:
    void HoldItem(AnyPtr<Item> item) {
        item_ = item;
    }
}
...


I still fail to see how offloading the implicit conversion to a new wrapper class is any different than just leaving the implicit conversion as-is?

You are taking the AnyPtr and storing a shared resource in a naked pointer. Yes that can be done, but that is not what AnyPtr is supposed to be used for.
Your HoldItem could also receive a SharedPtr and still save it incorrectly as an Item* by using the .Get() function. As I already said, in that case you should use a SharedPtr.

I think misusing AnyPtr in that way can't be prevented. ...
Oh wait I think it can: If the AnyPtr does also not have an implicit cast to T*. Instead it can be compared like a pointer and does also have operator*. It would still be constructable via T* and any SharedPtr<T> and whatever, so that functions taking an AnyPtr would accept any pointer as intended.
Is that the solution?
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by rku » 11 Apr 2016, 06:02

TheComet wrote:I'm saying it shouldn't be possible for this to compile:
Code: Select all
Item* item = Item::create(ITEM_FLOWER);



I stepped on this rake once too. Not doing that again though. Is it not too much fuss over non-issue really? I prefer cleaner and more convenient code to write. Having to .Get() all over the place will be less clean and convenient. Besides in modern IDEs return type is mouse-over-method away in modern IDEs. While forcing .Get() in various contexts is indeed correct way to do it in theory - in reality it is not that practical. It would be annoyance. Minor one but annoyance. And for what? To avoid a bug that is immediately visible? Not worth it imho.
User avatar
rku
Active user
Active user
 
Posts: 103
Joined: 06 May 2015, 08:24

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 11 Apr 2016, 19:29

The bug is not that immediately visible. Also I've made a suggestion how .Get() could be avoided and still be typesafe.
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by cadaver » 12 Apr 2016, 08:50

Texture2D::SetData does image = image->GetNextLevel(), which is runtime MIP calculation that returns a new image inside a new SharedPtr. In this case, forcing the user to pass a SharedPtr ensures the original image doesn't get destroyed within the function. It could be refactored to not manipulate the original image pointer, at the cost of slightly more complex code. In fact this would be preferable for more straightforward script bindings.

Terrain::SetHeightMap does nothing unusual, it just takes ownership of the passed image, in which case a raw ptr is sufficient. The Urho documentation states this of ownership in general:

"When an object's public API allows assigning a reference counted object to it through a Set...() function, this implies ownership through a SharedPtr. For example assigning a Material to a StaticModel, or a Viewport to Renderer. To end the assignment and free the reference counted object, call the Set...() function again with a null argument."

(http://urho3d.github.io/documentation/H ... tions.html)
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by cadaver » 13 Apr 2016, 10:59

Texturexxx::SetData() has been changed in master branch to use just a raw pointer. It will now rather manage the created MIP images internally by a different shared pointer.
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 20 Apr 2016, 09:48

I've decided to not go forth with this change. I didn't understand SharedPtr when I made this thread and now I do. It makes a lot of sense to allow implicit casting from and to raw pointers.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Previous

Return to Developer Talk

Who is online

Users browsing this forum: No registered users and 0 guests

cron