Re: Fix memory leak in LLVM (issue164084)

[col...@gmail.com]

PTAL


http://codereview.appspot.com/164084/diff/6/8
File include/llvm/Type.h (right):

http://codereview.appspot.com/164084/diff/6/8#newcode127
include/llvm/Type.h:127: virtual void destroy() const; // const is a
lie, this does "delete this"!
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> Note that virtual destroy() can still be private as long as subclasses
are only
> intended to override it and not call it.

Done.

http://codereview.appspot.com/164084/diff/6/8#newcode441
include/llvm/Type.h:441: protected:
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> Same here. These don't need to be protected unless subclasses need to
call them.

Done.

http://codereview.appspot.com/164084/diff/6/11
File lib/VMCore/LLVMContextImpl.h (right):

http://codereview.appspot.com/164084/diff/6/11#newcode214
lib/VMCore/LLVMContextImpl.h:214: static_cast<Type
*>(*I)->AbstractTypeUsers.clear();
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> Didn't it work to use "I->AbstractTypeUsers.clear()"? Subclasses
automatically
> have their base classes' fields.

Done.

http://codereview.appspot.com/164084/diff/6/10
File lib/VMCore/Type.cpp (right):

http://codereview.appspot.com/164084/diff/6/10#newcode1194
lib/VMCore/Type.cpp:1194: // concrete - this could potentially change us
from an abstract type to a
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> Since this is an opaque type, it couldn't change us from abstract to
concrete.

Done.

http://codereview.appspot.com/164084/diff/6/10#newcode1201
lib/VMCore/Type.cpp:1201: DerivedType::refineAbstractType(OldType,
NewType);
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> You're sure this is called? DerivedType doesn't define
refineAbstractType, and
> Type::refineAbstractType calls llvm_unreachable().

> Same for typeBecameConcrete.

Done.

http://codereview.appspot.com/164084/diff/6/10#newcode1210
lib/VMCore/Type.cpp:1210: void OpaqueType::destroy() const {
On 2009/12/04 02:55:01, Jeffrey Yasskin wrote:
> Can you put this inside Type::destroy()? As un-C++ish as that is, it
seems like
> that's the way LLVM has chosen to do this.

Done.

http://codereview.appspot.com/164084

[jyas...@gmail.com]

LGTM except for the nits below. Feel free to send upstream after fixing
them.


http://codereview.appspot.com/164084/diff/4005/5008
File lib/VMCore/Type.cpp (right):

http://codereview.appspot.com/164084/diff/4005/5008#newcode83
lib/VMCore/Type.cpp:83: else if (isa<OpaqueType>(this)) {
LLVM elses go on the same line as the '}'.

http://codereview.appspot.com/164084/diff/4005/5008#newcode85
lib/VMCore/Type.cpp:85: pImpl->OpaqueTypes.erase(static_cast<const
OpaqueType *>(this));
Instead of static_cast, you always want to use cast<> for LLVM types.
And then, since you have both isa<> and cast<>, you want to replace it
with dyn_cast<>:

} else if (const OpaqueType *opaque_this = dyn_cast<OpaqueType>(this)) {
LLVMontextImpl *pImpl = this->getContext().pImpl;
pImpl->OpaqueTypes.erase(opaque_this);
}

http://codereview.appspot.com/164084

[col...@gmail.com]

Sending upstream

http://codereview.appspot.com/164084/diff/4005/5008
File lib/VMCore/Type.cpp (right):

http://codereview.appspot.com/164084/diff/4005/5008#newcode83
lib/VMCore/Type.cpp:83: else if (isa<OpaqueType>(this)) {

On 2009/12/15 20:42:38, Jeffrey Yasskin wrote:
> LLVM elses go on the same line as the '}'.

Done.

http://codereview.appspot.com/164084/diff/4005/5008#newcode85
lib/VMCore/Type.cpp:85: pImpl->OpaqueTypes.erase(static_cast<const
OpaqueType *>(this));

On 2009/12/15 20:42:38, Jeffrey Yasskin wrote:
> Instead of static_cast, you always want to use cast<> for LLVM types.
And then,
> since you have both isa<> and cast<>, you want to replace it with
dyn_cast<>:

> } else if (const OpaqueType *opaque_this = dyn_cast<OpaqueType>(this))
{
> LLVMontextImpl *pImpl = this->getContext().pImpl;
> pImpl->OpaqueTypes.erase(opaque_this);
> }

Done.

http://codereview.appspot.com/164084