kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:198
llvm::StringMap<llvm::SmallVector<unsigned>> BySpelling;
+ llvm::StringMap<llvm::SmallVector<unsigned>> BySpellingAlternate;
llvm::DenseMap<const FileEntry *, llvm::SmallVector<unsigned>> ByFile;
----------------
maybe add a comment saying that these spellings are generated heuristically?
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:110-112
+ while (!P.empty() && path::is_separator(P.back()))
+ P.pop_back();
+ return P;
----------------
nit: `return P.rtrim('/');` // only separator we can encounter is forward slash
at this point.
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:129
ByLine[I.Line] = Index;
+ if (I.Resolved) {
+ ByFile[I.Resolved].push_back(Index);
----------------
nit: prefer early exit
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:137
+ // verbatim "e/f" should match (spelled=c/d/e/f, resolved=/a/b/c/d/e/f).
+ // We assume entry's (normalized) name will match the search dirs.
+ auto Path = normalizePath(I.Resolved->getName());
----------------
i think this assumption breaks when we have an absolute include search path,
which is a subdirectory of the CWD of file manager. as we might now have a
resolved include which is relative, but a search path that'll never match a
prefix of those includes.
i guess this is fine, as we're trying to heuristically match. but it would be
useful to mention it here and add a unittest for that. (unless I am missing
something and headersearch already normalizes such paths)
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:141
+ Parent = path::parent_path(Parent)) {
+ if (SearchPath.contains(Parent)) {
+ llvm::StringRef Rel = llvm::StringRef(Path).drop_front(Parent.size());
----------------
nit: prefer early exit
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:142-144
+ llvm::StringRef Rel = llvm::StringRef(Path).drop_front(Parent.size());
+ while (!Rel.empty() && path::is_separator(Rel.front()))
+ Rel = Rel.drop_front();
----------------
nit: `auto Rel = llvm::StringRef(Path).drop_front(Parent.size()).ltrim('/');`
// we already normalized the path and only have forward slashes.
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:171-173
+ for (unsigned I : BySpellingAlternate.lookup(Spelling))
+ if (!llvm::is_contained(Result, &All[I]))
+ Result.push_back(&All[I]);
----------------
i think we shouldn't look into alternate spellings if we've already found an
exact spelling. we would be matching wrong headers for sure in those cases.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155819/new/
https://reviews.llvm.org/D155819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
