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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
