saudi marked 2 inline comments as not done. saudi added inline comments.
================ Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:158-160 + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> PhysicalFileSystem( + llvm::vfs::createPhysicalFileSystem().release()); + RealFS = new llvm::vfs::ProxyFileSystem(PhysicalFileSystem); ---------------- dexonsmith wrote: > I think this can just be: > ``` > RealFS = llvm::vfs::createPhysicalFileSystem(); > ``` > The `ProxyFileSystem` wrapper is a convenience for building other file > systems; it doesn't do anything on its own IIRC. Note that createPhysicalFileSystem() returns a `std::unique_ptr` whereas `getRealFileSystem()` return type is `llvm::IntrusiveRfCntPtr`. Trying to change the type of RealFS requires more changes, as `DependencyScanningWorkerFilesystem` is itself a `ProxyFileSystem` and thus requires a `llvm::IntrusiveRfCntPtr` object. Should DependencyScanningWorkerFilesystem not be a ProxyFileSystem? @arphaman WDYT? However, in this case it might fall out of the scope of this patch, I'd suggest we keep it as a separate patch. I updated my patch for an intermediate fix, which also passes the tests: ``` RealFS = llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>(llvm::vfs::createPhysicalFileSystem().release()); ``` ================ Comment at: clang/test/ClangScanDeps/relative_directory.cpp:12-13 + +// The output order is non-deterministic when using more than one thread, +// so check the output using two runs. +// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck --check-prefix=CHECK1 %s ---------------- dexonsmith wrote: > Can you use `CHECK-DAG` for this? > https://www.llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive If I understand correctly from the doc, it would lose the order (CHECK1, CHECK1-NEXT). We need 2 blocks of 3 lines to be present, but only the blocks can be in any order. I did this mimicking what is done in other tests like regular_cdb.cpp. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91204/new/ https://reviews.llvm.org/D91204 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
