<div dir="ltr">+richard smith, who proposed to implement it this way<div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 13, 2013 at 6:08 PM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
I'm afraid this change is going to open a big can of worms. You are
essentially promote <br>
private member function to be public. Such practice should be
generally avoided even for <br>
the local/private class which is used in small scope, let alone
these fundamental structures <br>
which are extensively used in the compiler. <br><div>
<br>
> object needs the destructor called after
its memory was freed
<br>
<br></div>
If the block of memory containing APFloat/APInt is already freed,
how do you know <br>
the state of these objects are still valid? Depending on the
implementation, free-operation <br>
might clobber the memory with some special value for debugging
purpose. If it were safe<br>
to call needsCleanup(), why not directly call the destructor? Is
needsCleanup() sufficient <br>
and/or necessary condition for calling the destructor? How do we
keep them in sync in the <br>
future?<br></div></blockquote><div><br></div><div style>APFloat/APInt is placement new'ed in the code in question, and thus we need to call the destructors of any objects that do memory allocation themselves. We can save those destructor calls otherwise.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
I think you have to restructure the code that using APFloat/APInt to
avoid memory leak. <br>
I certainly understand that restructuring sometimes is not easy
undertaking, however, <br>
we have no other way around.<br>
<br>
BTW, by following the link, I take a glimpse of APValue, and spot a
potential problem there <br>
(I think it is irrelevant to the problem you are trying to tackle).
<br>
<br>
The union member "Data" will be cost to APFloat/APInt etc, and the
"Aligner" is the dummy <br>
field used to make sure the union sitting at right place. However,
how do we know <br>
the alignof(APFloat|APInt|etc) is no more stringent than
alignof(void*)?<br>
Shouldn't we use AlignedCharArrayUnion for this purpose?<br>
<br>
cat -n tools/clang/include/clang/AST/APValue.h<br>
38 class APValue {<br>
...<br>
<br>
116 union {<br>
117 void *Aligner;<br>
118 char Data[MaxSize];<br>
119 };</div></blockquote><div><br></div><div style>Richard wrote that code, and thus probably can explain the decision.</div><div style><br></div><div style>Cheers,</div><div style>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div><br>
<br>
<br>
<br>
<div>On 5/12/13 10:59 PM, Manuel Klimek
wrote:<br>
</div>
</div></div><blockquote type="cite"><div><div>
<pre>This is needed so one can check if the object needs the destructor called after
its memory was freed. See <a href="http://llvm-reviews.chandlerc.com/D736" target="_blank">http://llvm-reviews.chandlerc.com/D736</a> for where this
is useful.
<a href="http://llvm-reviews.chandlerc.com/D783" target="_blank">http://llvm-reviews.chandlerc.com/D783</a>
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;
}
</pre>
<br>
<fieldset></fieldset>
<br>
</div></div><pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div>
</blockquote></div><br></div></div>