[cfe-dev] [Patch] Make ParseAST() take ASTContext and TU params
Ted Kremenek
kremenek at apple.com
Tue Jan 27 15:42:34 PST 2009
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>
More information about the cfe-dev
mailing list