[PATCH] D134379: [clangd] IncludeCleaner: handle using namespace

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D134379#3808043 <https://reviews.llvm.org/D134379#3808043>, @kadircet wrote:

> In D134379#3807770 <https://reviews.llvm.org/D134379#3807770>, @ArcsinX wrote:
>
>> Anyway if this is the only concern, we can handle namespace declaration as a special case inside `ReferencedLocationCrawler::add()`. something like this:
>>
>>   diff
>>   -    for (const Decl *Redecl : D->redecls())
>>   -      Result.User.insert(Redecl->getLocation());
>>   +    if (llvm::isa<NamespaceDecl>(D)) {
>>   +      Result.User.insert(D->getCanonicalDecl()->getLocation());
>>   +    } else {
>>   +      for (const Decl *Redecl : D->redecls())
>>   +        Result.User.insert(Redecl->getLocation());
>>   +    }
>>
>> And in the above example `#include bar.h` will be suggested to remove
>>
>> Could this be a suitable solution?
>
> I don't think picking the canonical declaration would be helpful in general case; it'll likely preserve the first header all the time, which might be unused otherwise.
>
> I feel like the best option here is, diagnosing the unused using directive as well. but I am not sure if changing the lookup semantics would always be "right".

Diagnosing the unused using directive is a good option, but it's not related with `IncludeCleaner`. For now `IncludeCleaner` suggests to remove a header, but removing it leads to compile errors, which looks wrong.

> The other option would be to consider headers in batches after full analysis is complete, while keeping track of symbols provided by a particular header.
> That way when we have a symbol that's only re-declared in a bunch of headers, we can rank them based on other symbols being provided and only keep the headers that are providing some extra "used" symbols in addition to redecls.
> In the absence of such we get to pick an option between "all/none/forward declare ourselves". I guess picking "all" in this case is less troublesome, the user will get all the "unused include" warnings as soon as they use a symbol from that namespace.

This looks like a general problem, declaration of a used function in every header leads to "every header is used", maybe it's ok as far as it's not a common case.

So, yes, with the proposed solution we will get rid of some "false positive" cases, but we will get some "false negative" cases.
But IWYU handles this https://github.com/include-what-you-use/include-what-you-use/commit/97e236c55e5ebbd95b8bb221fd9497851f67c3cc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134379