[cfe-dev] [PATCH] Extend the use of DeclGroup

[Zhongxing Xu]

On Tue, Jan 6, 2009 at 2:26 AM, Ted Kremenek <kremenek at apple.com> wrote:

> Hi Zhongxing,
>
> 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:
>
>  TranslationUnit::~TranslationUnit() {
> +  for (std::vector<DeclGroupOwningRef>::reverse_iterator
> +         I = TopLevelDecls.rbegin(), E = TopLevelDecls.rend(); I != E;
> ++I) {
> +    I->clear();
> +  }
> +
> +#if 0
>   if (OwnsDecls) {
>     llvm::DenseSet<Decl*> Killed;
>     for (std::vector<Decl*>::reverse_iterator I=TopLevelDecls.rbegin(),
> @@ -102,7 +108,7 @@
>       (*I)->Destroy(*Context);
>     }
>   }
> -
> +#endif
>
> 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?


Yes.

 If so, there needs to be a big FIXME comment here documenting the issue.


Yes.

 Actually, I don't think we should have a regression in functionality here
> (i.e., releasing Decls) unless it is absolutely necessary.


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.


> 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.


May be similar code should be added to DeclGroupOwningRef::Destroy() to make
it work.


>
>
> On Jan 4, 2009, at 1:10 AM, Zhongxing Xu wrote:
>
>  This patch is the first step to solve the problem that no one is owning
>> the
>> struct decl in the following code:
>>
>> typedef struct s {} s_t;
>>
>> Later a TypeSpecifier holding this struct decl will be added to DeclGroup
>> as
>> discussed on the mailing list months before.
>>
>> The root of changes is at Sema::FinalizeDeclaratorGroup.  It returns
>> DeclGroupOwningRef instead of DeclTy*.  A bunch of related methods now
>> return
>> DeclGroupOwningRef instead of DeclTy*.
>>
>> Top level declarations are DeclGroups instead of Decls.  TranslationUnit
>> uses a vector of DeclGroupOwningRef to track all Decls.  Most of the dtor
>> of
>> TranslationUnit is disabled. The cleanup works should be done by the
>> DeclGroup.
>>
>> LinkageSpecDecl now uses a DeclGroup array to track all Decls.
>>
>> Three ObjC static analysis test cases fail. I haven't figured out the
>> reasons.
>>
>>
>> <dg.patch>_______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090106/7dcf0045/attachment.html>