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

[Alexei Svitkine]

Oops - it looks like there was a change to ParseAST() earlier that I
wasn't up to date with. This one:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090126/011412.html

That change involves propagating the FreeMemory param to the
ASTContext constructor... which is problematic with my change which
gets rid of that param entirely. I'm not sure how to handle this.

What is the purpose of -disable-free in the context of the
command-line clang tool?

-Alexei

On Tue, Jan 27, 2009 at 6:42 PM, Ted Kremenek <kremenek at apple.com> wrote:
> 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>
>
>