[PATCH] Introduce needsCleanup() for APFloat and APInt

[Shuxin Yang]

I'm afraid this change is going to open a big can of worms. You are 
essentially promote
private member function to be public. Such practice should be generally 
avoided even for
the local/private class which is used in small scope, let alone these 
fundamental structures
which are extensively used in the compiler.

 > object needs the destructor called after its memory was freed

If the block of memory containing APFloat/APInt is already freed, how do 
you know
the state of these objects are still valid? Depending on the 
implementation, free-operation
might clobber the memory with some special value for debugging purpose. 
If it were safe
to call needsCleanup(), why not directly call the destructor? Is 
needsCleanup() sufficient
and/or necessary condition for calling the destructor? How do we keep 
them in sync in the
future?

I think you have to restructure the code that using APFloat/APInt to 
avoid memory leak.
I certainly understand that restructuring sometimes is not easy 
undertaking, however,
we have no other way around.

BTW, by following the link, I take a glimpse of APValue, and spot a 
potential problem there
(I think it is irrelevant to the problem you are trying to tackle).

The union member "Data" will be cost to APFloat/APInt etc, and the 
"Aligner" is the dummy
field used to make sure the union sitting at right place. However, how 
do we know
the alignof(APFloat|APInt|etc) is no more stringent than alignof(void*)?
Shouldn't we use AlignedCharArrayUnion for this purpose?

cat -n tools/clang/include/clang/AST/APValue.h
38 class APValue {
...

116   union {
117     void *Aligner;
118     char Data[MaxSize];
119   };



On 5/12/13 10:59 PM, Manuel Klimek wrote:
> This is needed so one can check if the object needs the destructor called after
> its memory was freed. See http://llvm-reviews.chandlerc.com/D736 for where this
> is useful.
>
> http://llvm-reviews.chandlerc.com/D783
>
> Files:
>    include/llvm/ADT/APFloat.h
>    include/llvm/ADT/APInt.h
>    lib/Support/APFloat.cpp
>
> Index: include/llvm/ADT/APFloat.h
> ===================================================================
> --- include/llvm/ADT/APFloat.h
> +++ include/llvm/ADT/APFloat.h
> @@ -190,6 +190,10 @@
>       APFloat(const APFloat &);
>       ~APFloat();
>   
> +    bool needsCleanup() const {
> +      return partCount() > 1;
> +    }
> +
>       // Convenience "constructors"
>       static APFloat getZero(const fltSemantics &Sem, bool Negative = false) {
>         return APFloat(Sem, fcZero, Negative);
> Index: include/llvm/ADT/APInt.h
> ===================================================================
> --- include/llvm/ADT/APInt.h
> +++ include/llvm/ADT/APInt.h
> @@ -283,14 +283,18 @@
>   
>     /// @brief Destructor.
>     ~APInt() {
> -    if (!isSingleWord())
> +    if (needsCleanup())
>         delete [] pVal;
>     }
>   
>     /// Default constructor that creates an uninitialized APInt.  This is useful
>     ///  for object deserialization (pair this with the static method Read).
>     explicit APInt() : BitWidth(1) {}
>   
> +  bool needsCleanup() const {
> +    return !isSingleWord();
> +  }
> +
>     /// Profile - Used to insert APInt objects, or objects that contain APInt
>     ///  objects, into FoldingSets.
>     void Profile(FoldingSetNodeID& id) const;
> Index: lib/Support/APFloat.cpp
> ===================================================================
> --- lib/Support/APFloat.cpp
> +++ lib/Support/APFloat.cpp
> @@ -580,7 +580,7 @@
>   void
>   APFloat::freeSignificand()
>   {
> -  if (partCount() > 1)
> +  if (needsCleanup())
>       delete [] significand.parts;
>   }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130513/8298f122/attachment.html>