Hello everyone,
I bring to discussion the necessity/design of a new type of smart
pointer. r215176 and r217791 rise the problem, D5443
<http://reviews.llvm.org/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 <http://reviews.llvm.org/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
}
p->SomeMethod();
if (condition2) {
return nullptr; //
}
g(p.Get()); // don't take ownership of p
return p.StopOwn();
}
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.
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.
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140925/fb27d622/attachment.html>
-------------- next part --------------
//===-- 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;
}
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
Anton Yartsev <anton.yartsev at gmail.com> writes:> 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 pSo this takes ownership, but promises not to destroy the pointee in some way?> } > p->SomeMethod(); > > if (condition2) { > return nullptr; //I guess p is sometimes destroyed here, depending on condition1?> } > > g(p.Get()); // don't take ownership of p > return p.StopOwn(); > }What does it mean to stop owning the pointer twice? Doesn't this leak p in the case where condition1 was false?> 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.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.> 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; > }This seems to delete the pointer iff we *don't* own it. I think you have that backwards...> 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
On Wed, Sep 24, 2014 at 7:50 PM, Justin Bogner <mail at justinbogner.com> wrote:> Anton Yartsev <anton.yartsev at gmail.com> writes: > > 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 > > So this takes ownership, but promises not to destroy the pointee in some > way? > > > } > > p->SomeMethod(); > > > > if (condition2) { > > return nullptr; // > > I guess p is sometimes destroyed here, depending on condition1? > > > } > > > > g(p.Get()); // don't take ownership of p > > return p.StopOwn(); > > } > > What does it mean to stop owning the pointer twice? Doesn't this leak p > in the case where condition1 was false? > > > 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. > > 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. >Perhaps this is a case of poor examples, I'm not sure. I certainly share your concern that this might not be the right path forward, but it's something that I've seen come up repeatedly when doing unique_ptr migrations, perhaps moreso in Clang's frontend code. Essentially there are a variety of data structures that "sometimes" own their underlying data. Often implemented as a raw pointer and a boolean. (sometimes I've migrated this to a raw pointer and a unique_ptr - the raw pointer always points to the thing of interest, the unique_ptr is sometimes non-null and points to the same thing to keep it alive (or destroy it when it's no longer needed, more specifically)) I can go & dredge up some examples if we want to discuss the particular merits & whether each of those cases would be better solved in some other way, but it seemed pervasive enough in the current codebase that some incremental improvement could be gained by replacing these cases with a more specific tool for the job. We might still consider this tool to be "not ideal".> > > 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; > > } > > This seems to delete the pointer iff we *don't* own it. I think you have > that backwards... > > > 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 > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140924/4cbbce90/attachment.html>
On 25.09.2014 6:50, Justin Bogner wrote:> Anton Yartsev <anton.yartsev at gmail.com> writes: >> 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 > So this takes ownership, but promises not to destroy the pointee in some > way?Yes.>> } >> p->SomeMethod(); >> >> if (condition2) { >> return nullptr; // > I guess p is sometimes destroyed here, depending on condition1?Yes.> >> } >> >> g(p.Get()); // don't take ownership of p >> return p.StopOwn(); >> } > What does it mean to stop owning the pointer twice? Doesn't this leak p > in the case where condition1 was false?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() (r215176 <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f>, 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().>> 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. > 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.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. 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.>> 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; >> } > This seems to delete the pointer iff we *don't* own it. I think you have > that backwards...Right, thanks!> >> 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-- Anton -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140925/87884c2e/attachment.html>
Ping! Suggested is a wrapper over a raw pointer that is intended for freeing wrapped memory at the end of wrappers lifetime if ownership of a raw pointer was not taken away during the lifetime of the wrapper. The main difference from unique_ptr is an ability to access the wrapped pointer after the ownership is transferred. To make the ownership clearer the wrapper is designed for local-scope usage only. The main goal of the wrapper is to eliminate leaks like those in TGParser.cpp -r215176 <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f>. With the wrapper the fixes applied at r215176 <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f> could be refactored in the more easy and safe way. Attached is a proposed interface/implementation. Any ideas, suggestions? Is it OK to move forward with the solution?> Hello everyone, > > I bring to discussion the necessity/design of a new type of smart > pointer. r215176 and r217791 rise the problem, D5443 > <http://reviews.llvm.org/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 <http://reviews.llvm.org/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 > } > p->SomeMethod(); > > if (condition2) { > return nullptr; // > } > > g(p.Get()); // don't take ownership of p > return p.StopOwn(); > } > > 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. > > 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. > > -- > Anton-- Anton -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141002/a4a5bd1c/attachment.html> -------------- next part -------------- //===-- 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 #include <memory> namespace llvm { template<class T> class CondOwnershipPtr { T* Ptr; bool Own; void Delete() { if (Ptr && Own) delete Ptr; } CondOwnershipPtr(const CondOwnershipPtr&); // Prevent from misuse. CondOwnershipPtr& operator=(const CondOwnershipPtr&); // Prevent from misuse. public: CondOwnershipPtr() : Ptr(nullptr), Own(false) {} explicit CondOwnershipPtr(T* p) : Ptr(p) { Own = Ptr && true; } ~CondOwnershipPtr() { Delete(); } T* Get() const { return Ptr; } T* StopOwn() const { Own = false; return Ptr; } typename std::unique_ptr<T> MakeUnique() { assert((!Ptr || Own) && "Ownership was transferred."); return std::unique_ptr<T>(StopOwn()); } void Reset(T* P = nullptr) { if (P != Ptr) { Delete(); Ptr = P; Own = Ptr && true; } } bool Owns() const { return Ptr && Own; } operator bool() const { return Ptr != nullptr; } T& operator*() const { return *Ptr; } T* operator->() const { return Ptr; } }; } // end namespace llvm #endif
On Wed, Oct 1, 2014 at 3:14 PM, Anton Yartsev <anton.yartsev at gmail.com> wrote:> Ping! > > Suggested is a wrapper over a raw pointer that is intended for freeing > wrapped memory at the end of wrappers lifetime if ownership of a raw > pointer was not taken away during the lifetime of the wrapper. > The main difference from unique_ptr is an ability to access the wrapped > pointer after the ownership is transferred. > To make the ownership clearer the wrapper is designed for local-scope > usage only. >I'm still concerned this isn't the right direction, and that we instead want a more formal "maybe owning pointer". The specific use case you've designed for here, in my experience, doesn't seem to come up often enough to justify a custom ADT - but the more general tool of sometimes-owning smart pointer seems common enough (& problematic enough) to warrant a generic data structure (and such a structure would also be appliable to the TGParser use case where this came up). I'd love to hear some more opinions, but maybe we're not going to get them...> > The main goal of the wrapper is to eliminate leaks like those in TGParser.cpp > - r215176 > <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f>. > With the wrapper the fixes applied at r215176 > <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f> could > be refactored in the more easy and safe way. > > Attached is a proposed interface/implementation. > Any ideas, suggestions? Is it OK to move forward with the solution? > > Hello everyone, > > I bring to discussion the necessity/design of a new type of smart pointer. > r215176 and r217791 rise the problem, D5443 > <http://reviews.llvm.org/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 > <http://reviews.llvm.org/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 > } > p->SomeMethod(); > > if (condition2) { > return nullptr; // > } > > g(p.Get()); // don't take ownership of p > return p.StopOwn(); > } > > 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. > > 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. > > -- > Anton > > > > -- > Anton > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141001/55f9601f/attachment.html>