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

[Alexei Svitkine]

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ParseAST4.diff
Type: application/octet-stream
Size: 4531 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090127/eb4a0523/attachment.obj>