aganea accepted this revision. aganea added a comment. This revision is now accepted and ready to land.
LGTM, thank you! In D63907#1617417 <https://reviews.llvm.org/D63907#1617417>, @arphaman wrote: > Just for reference, this patch still doesn't reuse the FileManager across > invocations in a thread. We expect to get even better performance once we > reuse it, but I'm going have to improve its API first. Can't wait! @SamChaps is already testing this patch. He found some minor edge-cases with the minimizer (most likely unrelated to this), I'll post a patch. ================ Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76 +CachedFileSystemEntry +CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) { + assert(Stat.isDirectory() && "not a directory!"); ---------------- arphaman wrote: > aganea wrote: > > `llvm::vfs::Status &&Stat` to avoid a copy? > The copy should already be avoided, as I move the argument when passing in, > and when it's assigned to the result. If `Stat` is not a rvalue reference, wouldn't the `std::move` in the call site end up as a copy? See [[ https://godbolt.org/z/Rr7cdM | this ]]. If you change `int test(A a)` to `int test(A &&a)` you can see the difference in the asm output. However if the function is inlined, the extra copy will probably be optimized out. Not a big deal - the OS calls like stat() will most likely dominate here. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63907/new/ https://reviews.llvm.org/D63907 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
