<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Well, I didn't have concerns, and I
don't have concern either. <br>
I just personally abhor the practice that promote private func
(especially for the fundamental data structure)<br>
to public func without strong reasons. It is just my personal
taste. <br>
<br>
Please disregard my comment. If nobody else has objections, please
go ahead committing the patch. <br>
<br>
<br>
On 5/28/13 5:23 PM, Richard Smith wrote:<br>
</div>
<blockquote
cite="mid:CAOfiQqm2aD57nTqdgW-8A8Wxv6aVfXKq7bzhvrME_+L89AE=yA@mail.gmail.com"
type="cite">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 moz-do-not-send="true"
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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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 moz-do-not-send="true" href="http://llvm-reviews.chandlerc.com/D736" target="_blank">http://llvm-reviews.chandlerc.com/D736</a> for where this
is useful.
<a moz-do-not-send="true" 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 moz-do-not-send="true" href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a moz-do-not-send="true" 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 moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu"
target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
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 moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu"
target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
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>
</blockquote>
<br>
</body>
</html>