[PATCH] Introduce needsCleanup() for APFloat and APInt
Shuxin Yang
shuxin.llvm at gmail.com
Mon May 13 10:08:03 PDT 2013
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>
More information about the llvm-commits
mailing list