[cfe-dev] [Patch] Make ParseAST() take ASTContext and TU params

[Ted Kremenek]

Hi Alex,

Sorry for not looking at this sooner.  The patch looks good, but I  
have two additional nits.

First, could you possibly add a comment to the prototype and  
definition of ParseAST indicating what is the behavior of  
TranslationUnit and ASTContext are null?  The postcondition for this  
situation is that once ParseAST completes then any subsequent accesses  
to the ASTs it provided is no longer valid (because they are  
reclaimed).  This should be explicitly stated, since the ASTConsumer  
(which lives beyond the call to ParseAST) could potentially have  
dangling references to the ASTs.

Second, does it make sense to pass in both a TranslationUnit and an  
ASTContext pointer?  Since TranslationUnit holds a reference to  
ASTContext, I think we only need a single argument.  If the  
TranslationUnit* is null then the ASTContext* is null as well.

Ted

On Jan 27, 2009, at 11:49 AM, Alexei Svitkine wrote:

> If no one objects to these changes, can someone commit this to the  
> repository?
>
> -Alexei
>
> On Tue, Jan 27, 2009 at 10:31 AM, Alexei Svitkine
> <alexei.svitkine at gmail.com> wrote:
>> On Mon, Jan 26, 2009 at 2:29 PM, Alexei Svitkine
>> <alexei.svitkine at gmail.com> wrote:
>>> Here's an updated patch that also removes the comment about the
>>> FreeMemory param which no longer exists.
>>>
>>> -Alexei
>>>
>>> On Sat, Jan 24, 2009 at 2:51 PM, Alexei Svitkine
>>> <alexei.svitkine at gmail.com> wrote:
>>>> You are right. Here's a revised patch.
>>>>
>>>> -Alexei
>>>>
>>>> On Sat, Jan 24, 2009 at 11:19 AM, Sebastian Redl
>>>> <sebastian.redl at getdesigned.at> wrote:
>>>>> Alexei Svitkine wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Attached is a patch to make ParseAST() take ASTContext and
>>>>>> TranslationUnit as parameters, as suggested by Steve Naroff.
>>>>>>
>>>>>
>>>>> The double negative in the code confuses me somewhat, but isn't  
>>>>> this
>>>>> condition the wrong way round?
>>>>>
>>>>>> +    if (!DisableFree) {
>>>>>> +      Context = new ASTContext(PP.getLangOptions(),
>>>>>> PP.getSourceManager(),
>>>>>> +                               PP.getTargetInfo(),
>>>>>> +                               PP.getIdentifierTable(),
>>>>>> PP.getSelectorTable());
>>>>>> +      TU = new TranslationUnit(*Context);
>>>>>> +    }
>>>>>> +    ParseAST(PP, Consumer.get(), Context, TU, Stats);
>>>>>
>>>>> Previously, if DisableFree was true, FreeMemory was false, and  
>>>>> ParseAST
>>>>> didn't free the objects.
>>>>> Now, if DisableFree is true, the driver doesn't allocate  
>>>>> objects, so
>>>>> ParseAST does and will free them.
>>>>>
>>>>> Sebastian
>>>>>
>>>>
>>>
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev