[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report.Demo: https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/ecee6869e37af3db28089b64d8dce806/raw/8736e64c45af411e2c2d72adaed2dfc4410a5b36/ASTTests.html%25202

Fri Nov 18 13:11:08 PST 2022

sammccall added a comment.

In D138219#3936279 <https://reviews.llvm.org/D138219#3936279>, @hokein wrote:

> Thanks, I like the idea of marking missing-include refs.
>
> I haven't read the code yet, and below are all my findings when I played with the demo html (some of them are unrelated to the current patch, just want to dump all):

Sure thing, here are my notes :-)

> 1. size_t shows duplicated entries, line 465

This is because we're using the same decl-specifier with multiple declarators, but the AST doesn't model that. I don't think this is a big deal, we should deduplicate before actually displaying/applying findings but no need in this too imo.

> 2. we are missing refs in some using declarations (line 31, line32), but line 33 does have a link which seems weird

They are different types of symbol (template vs function), may make a difference?

> 3. UI related:
>   - it would be nice to highlight the whole line, if user clicks the line link from the shown-up panel;

Hmm, i think i can do this with CSS3 `:target`. Otherwise I don't think it's worth adding extra JS for.

> - for `Included path/to/header.h`, I think adding the `""`/`<>` around the spelling string will be nicer;

Agree, but we don't know which, and i don't think it's worth adding it to the library for that purpose only.

> - for main-file symbols, showing (`Header  ASTTests.cpp`) is suboptimal, but I don't have better solution;

I think `MainFile` should be a `Header` variant maybe. (Not just for this)

> 4. The handling of std symbols seems inconsistent:
>   - click on a vector `push_back` will give the mapping `vector` header;
>   - click on the type `std::vector` will give the `iosfwd` header;

At first glance this indeed looks like a bug (not in this patch i guess)

> 5. I was confused why the type `Annotation` has the `Included` file, but not the method call `.code()`, since both shown-up panel are showing `Annotations.h` header, then I realized that the `code` is from the base class `llvm::Annotation`. This brings up a policy question (Not sure it has been discussed before): If we have already included the header of a subclass, and we have some base-class method calls via the subclass object, do we need to insert the header of the base class?

Right, that's silly. I do think policy on member access in general is still a bit up in the air.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138219