[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

Tue Aug 6 11:51:37 PDT 2019

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