ArcsinX marked 3 inline comments as done. ArcsinX added a comment. As far as I do not have commit access, could you please commit for me? Aleksandr Platonov <platonov.aleksa...@huawei.com>
================ Comment at: clang/lib/Tooling/FileMatchTrie.cpp:111 + // As far as we do not support file symlinks we compare + // basenames here to avoid expensive request to file system. + if (llvm::sys::path::filename(Path) == ---------------- sammccall wrote: > Here it's really just for consistency - we have a single candidate, and > calling equivalent() on a single file isn't expensive. I'd be happy with or > without this check, but the comment should mention consistency. Removed "expensive" word. ================ Comment at: clang/lib/Tooling/FileMatchTrie.cpp:131 + // We failed to find the file with `Children.find()`. + // If `ConsumedLength` is equal to 0 then we have tried to find the file + // with it's basename. Thus, we do not have files with the same ---------------- sammccall wrote: > nit: This is quite a lot of words to come to "we do not support file > symlinks" - I think it could be a bit shorter and also get into motivation: > > "If `ConsumedLength` is zero, this is the root and we have no filename match. > Give up in this case, we don't try to find symlinks with different names". Fixed. 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
