[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

Tue Oct 1 11:58:59 PDT 2019

kousikk added a comment.

In D68193#1688838 <https://reviews.llvm.org/D68193#1688838>, @dexonsmith wrote:

> Sorry for bouncing you around, but I just had a look at the other user of `createFileEntry` and I think the right thing to do is to somehow share the code between `DependencyScanningWorkerFilesystem::status` and this.  I suggest splitting a function out of `status` (called `getOrCreateFileSystemEntry`?) that returns a `CachedFileSystemEntry` (or an error).
>
> The fix I just asked you to make (to only call `stat` once) could be done on `getOrCreateFileSystemEntry` as a follow-up, using `std::unique_ptr<llvm::vfs::File>` and changing `getFileEntry` to take that instead of a filename.

IIUC, 
`DependencyScanningWorkerFilesystem::status()` and `DependencyScanningWorkerFilesystem::openFileForRead()` both inturn call into `CachedFileSystemEntry::createFileEntry()`.

Given that, I made `getOrCreateFileEntry()` a private-member of `DependencyScanningWorkerFilesystem` and made both these functions use it first.

In a subsequent step, I passed in `llvm::vfs::Status` object to `createFileEntry()` instead of `Filename`. I hope this aligns with the code-organization you are looking for. Let me know if this aligns with how you wanted the code to look (happy to re-write if not).

I also ran it against a canonical set of tests I'm maintaining internally and I didn't notice a difference in either performance and didn't get new crashes of the tool, so I think there should NOT be any regressions because of this change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193