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