[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

[Douglas Gregor]

On Dec 1, 2009, at 11:18 PM, Chris Lattner wrote:
> On Dec 1, 2009, at 10:49 PM, Douglas Gregor wrote:
> 
>> Author: dgregor
>> Date: Wed Dec  2 00:49:09 2009
>> New Revision: 90300
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=90300&view=rev
>> Log:
>> Extend the source manager with the ability to override the contents of
>> files with the contents of an arbitrary memory buffer. Use this new
>> functionality to drastically clean up the way in which we handle file
>> truncation for code-completion: all of the truncation/completion logic
>> is now encapsulated in the preprocessor where it belongs
>> (<rdar://problem/7434737>).
> 
> Awesome, thanks for working on this.  Code completion specific stuff definitely didn't belong in SourceMgr.
> 
> To simplify discussion below, I'll break this into two pieces: a) handling of file truncation, and b) handling of code completion (liblex returning tok::codecomplete).   They are conceptually orthogonal.
> 
>> +++ cfe/trunk/lib/Basic/SourceManager.cpp Wed Dec  2 00:49:09 2009
>> @@ -41,66 +41,29 @@
>> 
>> +void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B) {
>> +  if (B == Buffer)
>> +    return;
> 
> It's a minor point, but I don't think this 'if' makes sense (it should be an assert(B!=Buffer)).  ContentCache takes ownership of the memory buffer.  If the pointer coming in was already owned by a ContentCache, then the client doesn't have ownership, so it can't pass it in.

Okay.

>> +  /// \brief Retrieve the memory buffer associated with the given file.
>> +  const llvm::MemoryBuffer *getMemoryBufferForFile(const FileEntry *File);
>> +
>> +  /// \brief Override the contents of the given source file by providing an
>> +  /// already-allocated buffer.
>> +  ///
>> +  /// \param SourceFile the source file whose contents will be override.
>> +  ///
>> +  /// \param Buffer the memory buffer whose contents will be used as the
>> +  /// data in the given source file.
>> +  ///
>> +  /// \returns true if an error occurred, false otherwise.
>> +  bool overrideFileContents(const FileEntry *SourceFile,
>> +                            const llvm::MemoryBuffer *Buffer);
> 
> 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

> 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? 

>> +++ cfe/trunk/include/clang/Lex/Preprocessor.h Wed Dec  2 00:49:09 2009
>> @@ -485,6 +488,27 @@
>>      CachedTokens[CachedLexPos-1] = Tok;
>>  }
>> 
>> +  /// \brief Specify the point at which code-completion will be performed.
>> +  ///
>> +  /// \param File the file in which code completion should occur. If
>> +  /// this file is included multiple times, code-completion will
>> +  /// perform completion the first time it is included. If NULL, this
>> +  /// function clears out the code-completion point.
>> +  ///
>> +  /// \param Line the line at which code completion should occur
>> +  /// (1-based).
>> +  ///
>> +  /// \param Column the column at which code completion should occur
>> +  /// (1-based).
>> +  ///
>> +  /// \returns true if an error occurred, false otherwise.
>> +  bool SetCodeCompletionPoint(const FileEntry *File, 
>> +                              unsigned Line, unsigned Column);
> 
> 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. It's not easy to see these connections. With the current API, at least all of the parts are together.

>> +
>> +  /// \brief Determine if this source location refers into the file
>> +  /// for which we are performing code completion.
>> +  bool isCodeCompletionFile(SourceLocation FileLoc);
> 
> This should be marked const?

Yup, thanks.

>> +++ 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.

	- Doug