<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 25.09.2014 6:50, Justin Bogner
wrote:<br>
</div>
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<pre wrap="">Anton Yartsev <a class="moz-txt-link-rfc2396E" href="mailto:anton.yartsev@gmail.com"><anton.yartsev@gmail.com></a> writes:
</pre>
<blockquote type="cite">
<pre wrap="">Hello everyone,
I bring to discussion the necessity/design of a new type of smart pointer.
r215176 and r217791 rise the problem, D5443 is devoted to the solution.
r215176 applies several temporary ugly fixes of memory leaks in TGParser.cpp
which would be great to be refactored using smart pointers. D5443 demonstrates
how the solution with a certain type of smart pointer would look like (see
changes in TGParser::ParseDef(), TGParser::InstantiateMulticlassDef() and
TGParser::ParseSimpleValue()).
Briefly:
consider a leaky example:
{
T* p = new T;
if (condition1) {
f(p); // takes ownership of p
}
p->SomeMethod();
if (condition2) {
return nullptr; // Leak!
}
g(p); // don't take ownership of p
return p;
}
The preferred solution would look like:
{
smart_ptr<T> p(new T);
if (condition1) {
f(p.StopOwn()); // takes ownership of p
</pre>
</blockquote>
<pre wrap="">
So this takes ownership, but promises not to destroy the pointee in some
way?</pre>
</blockquote>
Yes.
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<blockquote type="cite">
<pre wrap=""> }
p->SomeMethod();
if (condition2) {
return nullptr; //
</pre>
</blockquote>
<pre wrap="">
I guess p is sometimes destroyed here, depending on condition1?</pre>
</blockquote>
Yes.<br>
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> }
g(p.Get()); // don't take ownership of p
return p.StopOwn();
}
</pre>
</blockquote>
<pre wrap="">
What does it mean to stop owning the pointer twice? Doesn't this leak p
in the case where condition1 was false?</pre>
</blockquote>
The above example just illustrate the typical common patterns. By
the way, this particular example with several insignificant replaces
is much similar to the logic of TGParser::InstantiateMulticlassDef()
(<a
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f">r215176</a>,
memory is allocated at line 2406, at line 2457
Records.addDef(CurRec) takes ownership of CurRec). More examples
starting from line 2020, method TGParser::ParseDef().<br>
<br>
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<blockquote type="cite">
<pre wrap="">Neither unique_ptr nor shared_ptr can be used in the place of smart_ptr as
unique_ptr sets the raw pointer to nullptr after release() (StopOwn() in the
example above) whereas shared_ptr is unable to release.
</pre>
</blockquote>
<pre wrap="">
I don't understand why shared_ptr wouldn't suffice for the example
above. There are two cases in your example where you try to release the
pointer - in one of them it seems you don't actually want to release it,
because you continue using it after, and in the other the scope ends
immediately after the release. The shared_ptr releases when it goes out
of scope, so it seems to be exactly what you want here.
Maybe I'm just misunderstanding the use case here, but I fear that a
type like this one would just serve to paper over problems where the
ownership is very unclear, without really helping much.</pre>
</blockquote>
I agree the solution is not ideal, but it looks much safer to me
then just adding and switching extra boolean variables indicating
whether the memory should be freed or not and inserting missing,
sometimes conditional, 'delete's. With the suggested solution you
should only notice and call StopOwn() in places, where ownership is
transferred. <br>
The ownership logic is quite simple: "the wrapped memory should be
freed until otherwise stated". To be more clear I also intend to
design the wrapper for local scope usage only preventing it from
copiying/moving.<br>
<br>
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<blockquote type="cite">
<pre wrap="">Attached is a scratch that illustrates how the minimal
API/implementation of a desired smart pointer sufficient for
refactoring would look like. Any ideas and suggestions are
appreciated.
//===-- CondOwnershipPtr.h - Smart ptr with conditional release -*- C++ -*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//===----------------------------------------------------------------------===//
#ifndef LLVM_ADT_CONDOWNERSHIPPTR_H
#define LLVM_ADT_CONDOWNERSHIPPTR_H
namespace llvm {
template<class T>
class CondOwnershipPtr {
T* Ptr;
bool Own;
void Delete() {
if (Ptr && !Own)
delete Ptr;
}
</pre>
</blockquote>
<pre wrap="">
This seems to delete the pointer iff we *don't* own it. I think you have
that backwards...</pre>
</blockquote>
Right, thanks!<br>
<br>
<blockquote cite="mid:m2egv0ifbo.fsf@chronotis.apple.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">public:
CondOwnershipPtr() : Ptr(nullptr), Own(true) {}
explicit CondOwnershipPtr(T* p) : Ptr(p), Own(true) {}
~CondOwnershipPtr() {
Delete();
}
T* Get() const {
return Ptr;
}
T* StopOwn() const {
Own = false;
return Ptr;
}
void Reset(T* P = nullptr) {
if (P != Ptr) {
Delete();
Ptr = P;
Own = true;
}
}
bool Owns() const {
return Own;
}
operator bool() const {
return Ptr != nullptr;
}
T& operator*() const {
return *Ptr;
}
T* operator->() const {
return Ptr;
}
};
} // end namespace llvm
#endif
</pre>
</blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>