Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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):
- size_t shows duplicated entries, line 465
- we are missing refs in some using declarations (line 31, line32), but line 33 does have a link which seems weird
- UI related:
- it would be nice to highlight the whole line, if user clicks the line link from the shown-up panel;
- for Included path/to/header.h, I think adding the ""/<> around the spelling string will be nicer;
- for main-file symbols, showing (Header ASTTests.cpp) is suboptimal, but I don't have better solution;
- 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;
- 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?
Sure thing, here are my notes :-)
- 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.
- 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?
- 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)
- 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)
- 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.
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?
You're right, the template is the reason here. Taking a closing look, the logic of in WalkAST.cpp doesn't seem to handle well on this case.
E.g. for the following case, the UsingShadowDecl refers to the *primary* template decl, which is not marked as used or referenced. The used/referenced bit is only set for specialized FunctionDecl, so WalkAST doesn't report this UsingDecl location. (EnumDecl also has this problem).
namespace ns { template<typename T> int func(); } using ns::func; int k = func<int>();
(I do think the outstanding issues are not related to this patch, so this is ready for review)
That makes sense, I can imagine a few ways to fix this (change the bits in the AST, walk over specializations, drop the used||referenced approach).
Agree (sorry, I didn't mean we should address in this patch). We have found a few issues, I think we should record them in the issue tracker so that we don't lose them.
clang-tools-extra/include-cleaner/lib/HTMLReport.cpp | ||
---|---|---|
116 | nit: the FE can be removed -- we have the FileID, we can retrieve it by SM.getFileEntryByID(File) | |
126 | Worth to add a comment on the meaning of Satisfied. | |
160 | The current behaivor looks fine to me, but this is not the final state, right? We will revisit it when we have all the ranking/included-header bits in the library, if so, probably add a FIXME? | |
162 | a comment -- this is for the target symbol defined in the main file? |
clang-tools-extra/include-cleaner/lib/HTMLReport.cpp | ||
---|---|---|
116 | Sure, I'd just rather do that in the constructor than in the inner loop | |
160 | Ranking isn't relevant whether a ref is satisfied or not. If we add the concept of one provider dominating another, this logic might change. (e.g. for std::vector accept <iosfwd> but not if <vector> is included). But this is up in the air and also would probably be encapsulated in a signature change to Includes.match. Added a FIXME to avoid the brittle main-file check. | |
162 | Defined or declared or otherwise provided by the main file. |
clang-tools-extra/include-cleaner/lib/HTMLReport.cpp | ||
---|---|---|
115–116 | nit: maybe rename to MainFile to correspond to the MainFE |
nit: the FE can be removed -- we have the FileID, we can retrieve it by SM.getFileEntryByID(File)