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

[Ted Kremenek]

Hi Alexei,

Looks great!

I'm actually having difficulty applying the patch.  Did you generate  
it against top-of-tree clang?

Ted

On Jan 27, 2009, at 3:35 PM, Alexei Svitkine wrote:

> Oops some tabs made it into the previous one.
>
> Here's the same thing with correct whitespace.
>
> -Alexei
>
> On Tue, Jan 27, 2009 at 6:32 PM, Alexei Svitkine
> <alexei.svitkine at gmail.com> wrote:
>> Thanks for the comments!
>>
>> Here's a revised patch.
>>
>> -Alexei
>>
>> On Tue, Jan 27, 2009 at 6:22 PM, Ted Kremenek <kremenek at apple.com>  
>> wrote:
>>> Hi Alexei,
>>>
>>> I apologize, I left the "ei" off your name.
>>>
>>> On Jan 27, 2009, at 3:06 PM, Ted Kremenek wrote:
>>>
>>>> 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
>>>>
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>>
>>>
>>
> <ParseAST5.diff>