sammccall added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:24
+/// Indicates the relation between the reference and the target.
+enum class RefType {
----------------
as discussed I moved the other vocab types into types.h
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:26
+enum class RefType {
+ /// Target is explicit from the reference, e.g. function call.
+ Explicit,
----------------
named by the reference? or am I missing a case
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:31
+ /// Target's use can't be proven, e.g. a candidate for an unresolved
overload.
+ Ambigious,
+};
----------------
Ambiguous
================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:33
+/// Indicates the relation between the reference and the target.
+enum class RefType {
+ /// Target is explicit from the reference, e.g. function call.
----------------
kadircet wrote:
> hokein wrote:
> > I'm wondering what's our plan of supporting policy (different coding-style
> > may have different decisions about which includes are used)?
> >
> > IIUC, the RefType is part of the picture, providing fine-grained
> > information about each reference, and the caller can make their decisions
> > based on it?
> >
> > Thinking about the `Implicit` type, I think cases like non-spelled
> > constructor call, implicit conversion calls (maybe more?) fall into this
> > type, if we support `Policy.Constructor`, and `Policy.Conversion`, how do
> > we distinguish with these cases? We can probably do some ad-hoc checks on
> > the `TargetDecl`, but I'm not sure that the tuple `<SourceLocation,
> > TargetDecl, Ref>` will provide enough information to implement different
> > policy .
> > IIUC, the RefType is part of the picture, providing fine-grained
> > information about each reference, and the caller can make their decisions
> > based on it?
>
> Exactly that's the idea.
>
> > if we support Policy.Constructor, and Policy.Conversion, how do we
> > distinguish with these cases? We can probably do some ad-hoc checks on the
> > TargetDecl
>
> In our previous discussions we've also come to the conclusion that TargetDecl
> will have enough information for the caller to infer such information later
> on. We've decided to not make it part of the public api (at least initially)
> because it's unclear how widely useful those signals will be. But if it turns
> out to be needed by a variety of applications the design is extensible to
> either provide a:
> - inferDetails(Symbol) helper, which would derive a signal structure that
> extracts most of those signals needed, or
> - update Callback signature to also carry that information derived from the
> Symbol
>
> Does that make sense?
> In our previous discussions we've also come to the conclusion that TargetDecl
> will have enough information for the caller to infer such information later on
It was a while ago and my memory is bad, but this isn't my recollection (maybe
you're talking about different discussions). I think it was rather "this is the
bare minimum we need to get going, and we want to punt on real policy support
for now".
At least "TargetDecl will have enough information" seems implausible on its
face, e.g. how could you implement "types of variables are used, but types of
expressions are not used" based on inspecting TargetDecl?
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:154
void walkAST(Decl &Root, DeclCallback Callback) {
- ASTWalker(Callback).TraverseDecl(&Root);
+ bool IsHeader = Root.getASTContext().getLangOpts().IsHeaderFile;
+ ASTWalker(Callback, IsHeader).TraverseDecl(&Root);
----------------
(this is the first assumption in this file that we're traversing the main file.
Is it worth avoiding this by checking whether the root is written in the main
file?)
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:38
+ case RefType::Ambigious:
+ return "ambigious";
+ }
----------------
ambiguous
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:51
+void testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode,
+ bool IsHeader = false) {
llvm::Annotations Target(TargetCode);
----------------
tacking on boolean optional parameters to handle an uncommon option to a
function called everywhere doesn't scale well.
What about detecting the string `// header` in ReferencingCode?
Or even `// c++-header`, thinking ahead to objc etc
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:107
TEST(WalkAST, DeclRef) {
- testWalk("int ^x;", "int y = ^x;");
- testWalk("int ^foo();", "int y = ^foo();");
- testWalk("namespace ns { int ^x; }", "int y = ns::^x;");
- testWalk("struct S { static int ^x; };", "int y = S::^x;");
+ testWalk("int $explicit^x;", "int y = ^x;");
+ testWalk("int $explicit^foo();", "int y = ^foo();");
----------------
kadircet wrote:
> sammccall wrote:
> > This is very noisy. please find a way to avoid having to mark all test
> > cases as `$explicit`, e.g. by splitting implicit cases into different tests
> what about treating non-named points as explicit? that way we would only
> annotate non-explicit references (which are rare).
Not sure implicit references are actually rare, more like our examples are
trivial for now :-)
But I might be wrong, up to you.
(I would at least use $A^x instead of $ambiguous^x)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135859/new/
https://reviews.llvm.org/D135859
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
