[cfe-dev] Modifying an AST and Sema's dependency on an AST consumer

[steve naroff]

On Dec 4, 2008, at 12:04 PM, Ted Kremenek wrote:

> On Dec 4, 2008, at 8:42 AM, steve naroff <snaroff at apple.com> wrote:
>
>>
>> On Dec 4, 2008, at 11:27 AM, Ted Kremenek wrote:
>>
>>> On Dec 4, 2008, at 7:26 AM, steve naroff <snaroff at apple.com> wrote:
>>>
>>>>
>>>> On Dec 4, 2008, at 9:54 AM, Lukasz Janyst wrote:
>>>>
>>>>> On Wed, Dec 3, 2008 at 3:26 PM, Lukasz Janyst <ljanyst at cern.ch>  
>>>>> wrote:
>>>>>> tables in it. Yet still I seem to experience some strange memory
>>>>>> issues when the modified TU gets processed by the CodeGen.
>>>>>
>>>>> I sorted it out. The problem was the object ownership. It seemed
>>>>> natural to me that the identifier table should be owned by the
>>>>> ASTContext, but it is owned by the Preprocessor object which I  
>>>>> deleted
>>>>> after I was done with the parsing.
>>>>>
>>>>
>>>> The IdentifierTable should be owned by ASTContext (as you  
>>>> expected).
>>>>
>>>> This is a bug (that I thought we fixed a long time ago...apparently
>>>> not).
>>>>
>>>> snaroff
>>>
>>> I don't think this is a bug.  See my other email.
>>
>> Hey Ted,
>>
>> I read your email but I'm still not convinced there isn't an issue  
>> here (though it certainly isn't critical to deal with now).
>>
>> No doubt that running the preprocessor standalone will require an  
>> IdentifierTable.
>>
>> When a preprocessor is being run in a larger context (where AST's  
>> are being generated), it makes more sense (to me) if the "larger  
>> context" owns the IdentifierTable. In this instance, ASTContext.
>>
>> From my perspective, an instance of the Preprocessor is transient,  
>> while ASTContext is more long-lived.
>
> That's a fair argument, as a Preprocessor is more of a "worker"  
> object that is used to do a single pass over the source in a  
> translation unit.  One possible solution is to have the  
> IdentifierTable object be passed-by-reference to the constructor of  
> Preprocessor with the assumption that   the ownership of the  
> IdentifiedTable lies elsewhere.
>
> I'm not certain, however, what the performance implications would be  
> of such a change.  After mucking around in the Preprocessor for the  
> last  couple of weeks I'm convinced that even small changes can have  
> an unanticipated adverse effect on performance.

Understood. I don't think we need to act on this now.

snaroff