<br><br><div class="gmail_quote">On Tue, Jan 6, 2009 at 2:26 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi Zhongxing,<br>
<br>
Thanks for taking this on!  This is a big patch, so I'm taking some time to go through it.  I had a quick question though about one thing that caught my attention:<br>
<br>
 TranslationUnit::~TranslationUnit() {<br>
+  for (std::vector<DeclGroupOwningRef>::reverse_iterator<br>
+         I = TopLevelDecls.rbegin(), E = TopLevelDecls.rend(); I != E; ++I) {<br>
+    I->clear();<br>
+  }<br>
+<br>
+#if 0<br>
   if (OwnsDecls) {<br>
     llvm::DenseSet<Decl*> Killed;<br>
     for (std::vector<Decl*>::reverse_iterator I=TopLevelDecls.rbegin(),<br>
@@ -102,7 +108,7 @@<br>
       (*I)->Destroy(*Context);<br>
     }<br>
   }<br>
-<br>
+#endif<br>
<br>
It looks like that in your patch that Decls will not be freed in ~TranslationUnit since the "Destroy" method for each DeclGroupRef is not called.  Did you decide to do this because of issues with ownership?</blockquote>
<div> </div><div>Yes. <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">  If so, there needs to be a big FIXME comment here documenting the issue. </blockquote>
<div> </div><div>Yes.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Actually, I don't think we should have a regression in functionality here (i.e., releasing Decls) unless it is absolutely necessary.  </blockquote>
<div><br>I tried to Destroy() the DeclGroup in both order, but both crashed on some test cases. I think there may be bugs in the DeclGroupOwningRef's Destroy() method. I didn't investigate that further because I want to keep this patch as small as possible.<br>
 </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">While the code in ~TranslationUnit that bends over backwards to avoid deleting a Decl more than once is gross, it also works, and IMHO shouldn't be removed until we have cleaned up the ownership issues in the AST. </blockquote>
<div><br>May be similar code should be added to DeclGroupOwningRef::Destroy() to make it work.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c"><br>
<br>
On Jan 4, 2009, at 1:10 AM, Zhongxing Xu wrote:<br>
<br>
</div></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="Wj3C7c">
This patch is the first step to solve the problem that no one is owning the<br>
struct decl in the following code:<br>
<br>
typedef struct s {} s_t;<br>
<br>
Later a TypeSpecifier holding this struct decl will be added to DeclGroup as<br>
discussed on the mailing list months before.<br>
<br>
The root of changes is at Sema::FinalizeDeclaratorGroup.  It returns<br>
DeclGroupOwningRef instead of DeclTy*.  A bunch of related methods now return<br>
DeclGroupOwningRef instead of DeclTy*.<br>
<br>
Top level declarations are DeclGroups instead of Decls.  TranslationUnit<br>
uses a vector of DeclGroupOwningRef to track all Decls.  Most of the dtor of<br>
TranslationUnit is disabled. The cleanup works should be done by the DeclGroup.<br>
<br>
LinkageSpecDecl now uses a DeclGroup array to track all Decls.<br>
<br>
Three ObjC static analysis test cases fail. I haven't figured out the reasons.<br>
<br>
<br></div></div>
<dg.patch>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</blockquote>
<br>
</blockquote></div><br>