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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
