[PATCH] Introduce needsCleanup() for APFloat and APInt
Manuel Klimek
klimek at google.com
Wed May 29 00:04:33 PDT 2013
On Wed, May 29, 2013 at 4:04 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
> Well, I didn't have concerns, and I don't have concern either.
> I just personally abhor the practice that promote private func (especially
> for the fundamental data structure)
> to public func without strong reasons. It is just my personal taste.
>
> Please disregard my comment. If nobody else has objections, please go
> ahead committing the patch.
>
Richard, is that enough "consensus"?
>
>
> On 5/28/13 5:23 PM, Richard Smith wrote:
>
> 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/20130529/5c0fb1dc/attachment.html>
More information about the llvm-commits
mailing list