[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: chandlerc.
sammccall added a comment.

In D83621#2152618 <https://reviews.llvm.org/D83621#2152618>, @ArcsinX wrote:

> In D83621#2151716 <https://reviews.llvm.org/D83621#2151716>, @sammccall wrote:
>
> > In D83621#2146750 <https://reviews.llvm.org/D83621#2146750>, @ArcsinX wrote:
> >
> > > > - don't scan for equivalences if the set of candidates exceeds some size threshold (10 or so)
> > > > - don't scan for equivalences if the node we'd scan under is the root
> > >
> > > After such fixes `JSONCompilationDatabase::getCompileCommands()` will return other results than before in some cases.
> >
> >
> > Can you elaborate on this - do you think there are reasonable cases where it will give the wrong answer?
> >  I can certainly imagine cases where this changes behaviour, e.g. project/foo.cc is a symlink to project/bar.cc, compile_commands contains an entry for foo.cc, we query for bar.cc.
> >  But I don't think this was ever really intended to handle arbitrary symlinks that change the names of files, and I do think this is OK to break.
>
>
> If breaking `project/foo.cc <=> project/bar.cc` is OK, then could we also break `some/path/here/foo.cc <=> some/other/path/bar.cc`?


Yes. We discussed offline a bit with @chandlerc who doesn't think identifying cases where symlinked files have different names was a motivating case or likely to be important.

> In that case we can just prevent `Comparator.equivalent(FileA,FileB)` calls when `llvm::sys::path::filename(FileA) != llvm::sys::path::filename(FileB)`.

You can - but this is equivalent to not doing the `getAll()` path when the node is the root (i.e. when ConsumedLength == 0).
So I think `if (ConsumedLength == 0) return {}` around line 124 in FileMatchTrie with an appropriate comment would also do the trick, and avoid pointless traversal/copy/string comparisons (much cheaper than IO but not free).

> I tried this locally:
> 
> - it works fast.
> - does not break tests.
> - easy to add unit test for `FileMatchTrie`.
> 
>   What do you think?

+1, happy with this behavior.
I think we should try the implementation that skips the traversal first - it's no more code and should be a bit more efficient.
But either way works, really.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83621