Fix memory leak in LLVM

Fix the memory leak observed by the heapchecker. All LLVM and Clang tests pass.

Sent upstream as http://codereview.appspot.com/180073.

[Collin Winter, 2009-12-03 23:20:38]

PTAL

[Jeffrey Yasskin, 2009-12-04 02:55:01]

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"!
Note that virtual destroy() can still be private as long as subclasses are only
intended to override it and not call it.

http://codereview.appspot.com/164084/diff/6/8#newcode441
include/llvm/Type.h:441: protected:
Same here. These don't need to be protected unless subclasses need to call them.

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();
Didn't it work to use "I->AbstractTypeUsers.clear()"? Subclasses automatically
have their base classes' fields.

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
Since this is an opaque type, it couldn't change us from abstract to concrete.

http://codereview.appspot.com/164084/diff/6/10#newcode1201
lib/VMCore/Type.cpp:1201: DerivedType::refineAbstractType(OldType, NewType);
You're sure this is called? DerivedType doesn't define refineAbstractType, and
Type::refineAbstractType calls llvm_unreachable().

Same for typeBecameConcrete.

http://codereview.appspot.com/164084/diff/6/10#newcode1210
lib/VMCore/Type.cpp:1210: void OpaqueType::destroy() const {
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.

[Collin Winter, 2009-12-15 19:24:12]

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.

[Jeffrey Yasskin, 2009-12-15 20:42:38]

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);
}

[Collin Winter, 2009-12-16 19:26:09]

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.