[PATCH] D155819: [include-cleaner] Loose matching for verbatim headers

[Kadir Cetinkaya via Phabricator via cfe-commits]

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