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?<br><br><div class="gmail_quote">On Mon, May 27, 2013 at 8:28 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Ping.</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, May 21, 2013 at 11:19 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Ping.</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Wed, May 15, 2013 at 9:58 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Mon, May 13, 2013 at 10:08 AM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_quote"><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'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>
<br>
> object needs the destructor called after
its memory was freed
<br>
<br>
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><div>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.</div>
<div><div>
<div><br></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 };<br>
<br>
<br>
<br>
<div>On 5/12/13 10:59 PM, Manuel Klimek
wrote:<br>
</div>
<blockquote type="cite">
<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>
<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>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div></div></div><br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br>