[PATCH] Introduce needsCleanup() for APFloat and APInt
Shuxin Yang
shuxin.llvm at gmail.com
Tue May 28 19:04:16 PDT 2013
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.
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
> <mailto:klimek at google.com>> wrote:
>
> Ping.
>
>
> On Tue, May 21, 2013 at 11:19 AM, Manuel Klimek <klimek at google.com
> <mailto:klimek at google.com>> wrote:
>
> Ping.
>
>
> On Wed, May 15, 2013 at 9:58 PM, Richard Smith
> <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>
> On Mon, May 13, 2013 at 10:08 AM, Shuxin Yang
> <shuxin.llvm at gmail.com <mailto: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. Seehttp://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 <mailto: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 <mailto: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 <mailto: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/03039ef3/attachment.html>
More information about the llvm-commits
mailing list