[PATCH] Introduce needsCleanup() for APFloat and APInt

[Richard Smith]

Well, the patch LGTM, but I'd like some indication that we have reached
consensus before you commit. Shuxin, do you still have concerns here?

On Mon, May 27, 2013 at 8:28 AM, Manuel Klimek <klimek at google.com> wrote:

> Ping.
>
>
> On Tue, May 21, 2013 at 11:19 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> Ping.
>>
>>
>> On Wed, May 15, 2013 at 9:58 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Mon, May 13, 2013 at 10:08 AM, Shuxin Yang <shuxin.llvm at gmail.com>wrote:
>>>
>>>>  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 Manuel's phrasing here may be throwing you. What we want is a
>>> mechanism to determine whether the destructor would be trivial (and thus we
>>> don't need to bother cleaning the object up). We are *not* calling a
>>> destructor after the memory was freed.
>>>
>>> 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 listllvm-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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/20130528/3db19a4a/attachment.html>