[PATCH] Some infrastructure work for virtual file system (now on phab)

Rafael Ávila de Espíndola rafael.espindola at gmail.com
Wed Feb 12 14:21:18 PST 2014



================
Comment at: include/clang/Basic/FileManager.h:122
@@ -121,2 +121,3 @@
 class FileManager : public RefCountedBase<FileManager> {
+  IntrusiveRefCntPtr<AbstractFileSystem> FS;
   FileSystemOptions FileSystemOpts;
----------------
Why is the reference count necessary? Given its nature I would expect the FS to outlive the file manger, in which case the FileManager could have just a pointer to the FileSystem.

================
Comment at: include/clang/Basic/VirtualFileSystem.h:62
@@ +61,3 @@
+    /// @{
+    llvm::sys::fs::file_type type() const { return Type; }
+    llvm::sys::fs::perms permissions() const { return Perms; }
----------------
It might be better to use llvm's names: getType for example.

================
Comment at: include/clang/Basic/VirtualFileSystem.h:130
@@ +129,3 @@
+  /// @{
+  llvm::error_code status(FileDescriptor FD, Status &Result);
+  llvm::error_code getBufferForFile(FileDescriptor FD, const llvm::Twine &Name,
----------------
Please upgrade this to return ErrorOr<Status>. We should really upgrade sys::fs, but at least don't propagate it further.



http://llvm-reviews.chandlerc.com/D2745



More information about the cfe-commits mailing list