[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Duncan, thanks for working on better interfaces in clang!

I am just wondering - is it safe to have the lifetime of a single object on heap managed by two different `IntrusiveRefCntPtr` instances?



================
Comment at: clang/lib/Frontend/ASTUnit.cpp:1693
                                       PrecompilePreambleAfterNParses,
-                                      AST->FileMgr->getVirtualFileSystem()))
+                                      &AST->FileMgr->getVirtualFileSystem()))
     return nullptr;
----------------
Is this safe?


================
Comment at: clang/lib/Frontend/ASTUnit.cpp:1800
     assert(FileMgr && "FileMgr is null on Reparse call");
-    VFS = FileMgr->getVirtualFileSystem();
+    VFS = &FileMgr->getVirtualFileSystem();
   }
----------------
Is this safe?


================
Comment at: clang/lib/Frontend/ASTUnit.cpp:2236
 
-    auto VFS = FileMgr.getVirtualFileSystem();
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS =
+        &FileMgr.getVirtualFileSystem();
----------------
Is this safe?


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:299
   if (!VFS)
-    VFS = FileMgr ? FileMgr->getVirtualFileSystem()
+    VFS = FileMgr ? &FileMgr->getVirtualFileSystem()
                   : createVFSFromCompilerInvocation(getInvocation(),
----------------
Is this safe?


================
Comment at: clang/lib/Tooling/Tooling.cpp:304
   const std::unique_ptr<driver::Driver> Driver(
-      newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
+      newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.
----------------
The `Driver` constructor takes `IntrusiveRefCntPtr< llvm::vfs::FileSystem >` as an argument. Is this safe?

https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#a1930cae44e647c1983d11d6a61ce14ed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59388/new/

https://reviews.llvm.org/D59388