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