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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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".
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 is not easy in the current implementation of clangd, as we preserve symbols provided by headers, but the new include-cleaner library we're working on is more accommodating for such logic. Hence I'd rather leave this implementation as-is today, and see if such a behaviour would be desired in general (not only for namespace-decls, but pretty much any declaration for which we can only see "forward declarations" without a clear "canonical declaration"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134379