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

Thu Jul 27 03:01:40 PDT 2023

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.

================
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]);
----------------
sammccall wrote:
> kadircet wrote:
> > 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.
> I had it this way initially and changed my mind. Having the presence of some includes affect whether others match at all breaks an "obvious" part of the contract. (So obvious that I assumed it in the test ten minutes after deliberately breaking it). I wouldn't add a special case here unless there's some observed bad-results motivation for it.
> 
> > we would be matching wrong headers for sure in those cases.
> 
> You could be including the same header under two paths (and in fact that's what this testcase is doing). I don't know why you would, but we also don't have real examples of this happening with *different* headers.
> You could be including the same header under two paths (and in fact that's what this testcase is doing). I don't know why you would

I do think it's better to diagnose one of those as unused in such a scenario. As tools like clang-format won't be able to de-duplicate those automatically.

but you're right about not having examples for alternate spellings resolving to different files, so I guess this is at least "safe" and we can change the behaviour if we encounter examples.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155819