Removed potentially-misleading dead code and comment

[mqudsi]

Since ptr is being passed by value to the lambda expression,
setting ptr to null will have no effect past that single line.

The combination of the code and the comment which implies there
is an additional benefit/security in doing so might mislead a
maintainer or developer down the road.

[aminroosta]

Thanks for your contribution 👍

[Killili]

Hey i want my nullptr back! ;)

[mqudsi]

@aminroosta Cheers.

@Killili yes, but the pointer is being passed by value, not by reference. As in, the ptr you are nulling here is only nulled in this scope and not anywhere else. I suppose you could change it to [=](sqlite3* &ptr) for the nulling to have effect (then you can get your nullptr back and your comment too, to boot), but I don't know if that'll compile without some wrangling or not.

[aminroosta]

I think [](sqlite3* &ptr) { prt = null; } is a bad idea, because it will change the internal memory of shared_ptr.
I think we shouldn't worry about setting ptr to null at all, because when shared_ptr destructor is called
we are sure that no-one has a reference to it any more.

[aminroosta]

@Killili do you think [&_db](sqlite3* ptr) { sqlite3_close_v2(ptr); _db = nullptr; } is a good idea?
I am not sure :-)

[mqudsi]

@aminroosta Just in case it looked like I was endorsing that option - I wasn't. Just pointing out what the code would have had to look like in order to match the intent described in the (now deleted) comment.

[Killili]

There was something about referencing lambdas as custom deleters and "why thats bad". Thats why its a [=]{} lambda.

[Killili]

Thats so typical , I write one comment and someone finds that something is wrong with it ;)

@mqudsi youre right about it not making any difference but it had a purpose and i think we should have it. Because the people using the shared_ptr outside of our class will use it in its naked untracked form as in some_function( ptr.get() , ...) and maybe dont realize that when they go out of scope of the shared_ptr.

[mqudsi]

@Killili I'm not sure I understand; I'm a little confused. The code that was there had no effect and did not change the value of ptr.get(). In fact, the code was probably elided by the compiler as it assigns a value to an object just before that ephemeral object goes out of scope. I don't see how that would help anyone, unless you are actually referring to the lambda-with-pointer-by-value that code sample I wrote in my earlier comment?

[Killili]

@mqudsi As i said you where right. It did nothing there in its last version but i had it there intentionally to do something that i think is worth having. And now i'm trying to get it actually working in the why it was intended.

[mqudsi]

@Killili thanks, that makes sense.

[aminroosta]

When a user uses auto p = ptr.get() somewhere,
then even if we set _db = nullptr (or something like the previous code).
Users will still have a copy of the original pointer. right?

So even if we set the actual shared_ptr to nullptr it won't help either.

@Killili I think if it makes a difference while debugging in visual studio, then it won't hurt to have it back.
with and extra comment.
I will send another PR.

[Killili]

@aminroosta no the code will still do nothing as @mqudsi explained and the difference in debug behavior is due to shared_ptr nulling itself on destruction in debug but saving this op at runtime (at least in VC last time i checked).

[aminroosta]

I think we can all live with an extra comment :-)
Done in aminroosta@81bfd0f

[Killili]

Nice but this actually makes it worse ;)

[aminroosta]

Why? Is that sarcastic :-)

[Killili]

Now we have a comment saying that this line does nothing and it does not even do the thing it is supposed to help with...

[aminroosta]

@Killili
1- [](sqlite3& *ptr) { ptr = nullptr; }
2- [&_db] (sqlite3 * ptr) { ...; _db = nullptr; }

which one? do think is the better approach.

[Killili]

Ok i tested a few scenarios and we can't possibly follow the copy's of the naked pointer. So i think we should just drop the thing and call it a day.

The line should just be:

_db = std::shared_ptr<sqlite3>(tmp, [=](sqlite3* ptr) { sqlite3_close_v2(ptr); }); // this will close the connection eventually when not longer needed, and can not solve world hunger in the process!

[Killili]

1- will do absolutely nothing
2- will potential corrupt the stack
so both are a no go

[aminroosta]

@Killili i agree.
Done in aminroosta@0f46197