[cfe-commits] r90300 - in /cfe/trunk: include/clang/Basic/SourceManager.h include/clang/Lex/Lexer.h include/clang/Lex/Preprocessor.h lib/Basic/SourceManager.cpp lib/Frontend/CompilerInstance.cpp lib/Lex/Lexer.cpp lib/Lex/Preprocessor.cpp

[Chris Lattner]

On Dec 1, 2009, at 11:43 PM, Douglas Gregor wrote:

>>
>> This API is somewhat unfortunate.  It doesn't make a lot of sense  
>> (to me) for SourceMgr to load a file, then have to 'override' it.   
>> It seems that libfrontend (the only thing that should know about  
>> "truncation" IMO) should call into SourceMgr (potentially before  
>> the preprocessor is even constructed!) with a  
>> "setMemoryBufferForFileEntry" call.  This call would assert if the  
>> FileEntry already had a MemoryBuffer.
>
> It can't assert. The second client of overrideFileContents (not  
> committed yet) allows us to replace the contents of a FileEntry*  
> with the contents of a totally unrelated FileEntry* from the command  
> line (e.g., "pretend that a.cpp has the contents of /tmp/a.cpp").  
> These contents may then be overridden again by truncation for code- 
> completion purposes. I don't want to force clients to tip-toe around  
> double-replacements

Ok, works for me!

>> The nice thing about this is that we hoist all the handling of  
>> "truncation" completely out of libbasic and liblex.  The only thing  
>> we need to add is a single (simple) SourceManager API  
>> (setMemoryBufferForFileEntry or whatever you decide to call it).
>
> Aside from assert vs. replacing an existing buffer, we're talking  
> about the same simple API but with two different names  
> (overrideFileContents or setMemoryBufferForFileEntry), right?

Yes, if you need 'override' for the later client, than keeping it the  
way it is is fine.  I was just trying to 'strength reduce' the API.

>> This should be moved to libfrontend, I consider this part of the  
>> 'truncation' half of this.
>
> I kinda sorta agree, but moving the truncation bits to libfrontend  
> splits the low-level mechanism for code completion (truncation + EOF  
> translation) into two almost completely unrelated parts of the  
> compiler.

Right, that's the point though: these are orthogonal concepts which  
happen to be used together (today) for one use.  I don't think that  
two orthogonal pieces being in different places is a bad thing at all.

>>> +++ cfe/trunk/lib/Lex/Lexer.cpp Wed Dec  2 00:49:09 2009
>>> @@ -1326,24 +1321,16 @@
>>> // Otherwise, check if we are code-completing, then issue  
>>> diagnostics for
>>> // unterminated #if and missing newline.
>>>
>>> +  if (PP && PP->isCodeCompletionFile(FileLoc)) {
>>
>> I'm fine with a hook being here to turn EOF ->  
>> tok::code_completion.  It would be slightly cleaner conceptually to  
>> have the 'IsEofCodeCompletion' bool back, to prevent having to poke  
>> the preprocessor to get this state, but I'm fine with it either way.
>
> Even when we had IsEofCodeCompletion, we had to poke the  
> preprocessor because the lexer sees many EOFs... only the one for  
> the CodeCompletionFile actually translates into  
> tok::code_completion. Adding isCodeCompletionFile into the  
> preprocessor centralized the logic.

Right, the PP would have to only set this bit for the "last" file.   
I'm fine keeping it the way you have it, no worries.

>>
>> This should be moved to libfrontend, I consider this part of the  
>> 'truncation' half of this.
>
>
> I just had a "Duh!" moment about this... the preprocessor needs to  
> know about code completion anyway, because we should be able to  
> complete things like:
>
> 	#<list of preprocessor directives>
> 	#if defined(<list of things that are defined>
> 	#pragma <known pragmas>
>
> 	#define FOO(X, Y, Z)
> 	FOO(blah, <completion containing Y, Z)>
>
> all of which are in the domain of the preprocessor. We just haven't  
> tried to implement any code completions in the preprocessor yet.

Again, I'm not opposed to lex/pp knowing about code completion, I  
don't want them to know about truncation.

-Chris