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

> const llvm::MemoryBuffer *ContentCache::getBuffer(std::string *ErrorStr) const {
>   // Lazily create the Buffer for ContentCaches that wrap files.
>   if (!Buffer && Entry) {
>     // FIXME: Should we support a way to not have to do this check over
>     //   and over if we cannot open the file?
>     Buffer = MemoryBuffer::getFile(Entry->getName(), ErrorStr,Entry->getSize());
> -    if (isTruncated())
> -      const_cast<ContentCache *>(this)->truncateAt(TruncateAtLine, 
> -                                                   TruncateAtColumn);

Woo hoo :)

...

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

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


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

> +
> +  /// \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?

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

> +bool Preprocessor::SetCodeCompletionPoint(const FileEntry *File, 
> +                                          unsigned TruncateAtLine, 
> +                                          unsigned TruncateAtColumn) {

This is the logic that I think should move to libfrontend.

Thanks again for working on this Doug, this patch is a huge improvement!

-Chris