patch for improving Clang's virtual file handling

Hi, Doug,

Per our chat, please review the attached patch Manuel and I wrote for
improving Clang's virtual file handling. I also uploaded the patch to
http://codereview.appspot.com/4126060 to make it easier to comment on
the individual lines.

This patch contains:

- making some of the existing comments more accurate in the presence
of virtual files/directories.

- renaming some private data members of FileManager to match their roles better.

- creating 'DirectorEntry's for the parent directories of virtual
files, such that we can tell whether two virtual files are from the
same directory. This is useful for injecting virtual files whose
directories don't exist in the real file system.

- minor clean-ups and adding comments for class
FileManager::UniqueDirContainer and FileManager::UniqueFileContainer.

- adding statistics on virtual files to FileManager::PrintStats().

I didn't add new tests, but all existing tests pass. I plan to add
the tests in a new patch, as I'm still figuring out how to do that.

Thanks a lot!

virtual-dir.patch (20.4 KB)

Hi Zhanyong,

Your patch looks good to me. Did you consider using the functionality in llvm::sys::path to handle the parsing of path names, rather than hand-coding string algorithm such as

+void FileManager::addAncestorsAsVirtualDirs(llvm::StringRef Path) {
+ size_t SlashPos = Path.size();

Thanks, Doug!

Hi Zhanyong,

Your patch looks good to me. Did you consider using the functionality in llvm::sys::path to handle the parsing of path names, rather than hand-coding string algorithm such as

+void FileManager::addAncestorsAsVirtualDirs(llvm::StringRef Path) {
+ size_t SlashPos = Path.size();
+
+ // Find the beginning of the last segment in Path.
+ while (SlashPos != 0 && !IS_DIR_SEPARATOR_CHAR(Path[SlashPos-1]))
+ --SlashPos;
+
+ // Ignore repeated //'s.
+ while (SlashPos != 0 && IS_DIR_SEPARATOR_CHAR(Path[SlashPos-1]))
+ --SlashPos;
+
+ if (SlashPos == 0)
+ return;

I wasn't aware of llvm::sys::path, so I just followed what the
existing code in FileManager was doing. I like the idea of switching
to llvm::sys::path.

May I commit the patch as is and send you a separate one for using
llvm::sys::path? Thanks,

Yes, that’s fine. Thanks!

  • Doug