ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+ Ref = ReferenceLoc{
+ E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+ }
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > I believe it is better to return `getFoundDecl` then `getDecl`, the
> > > former respects using declarations.
> > Good point. Done. Added a test too.
> I was actually referring to `DeclRefExpr`, change seems to be on `MemberExpr`
Good point. Fixed that one too, thanks!
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:548
+ bool TraverseElaboratedTypeLoc(ElaboratedTypeLoc L) {
+ // ElaboratedTypeLoc will reports information for its inner type loc.
+ // Otherwise we loose information about inner types loc's qualifier.
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > why not just traversenestednamespecifier and `visitNode(L)` instead of
> > > calling the base::traverse ?
> > To avoid re-implementing the traversal logic. `Base::Traverse` does exactly
> > that, we shouldn't re-implement traversal ourselves.
> I see but that should help get rid of `TypeLocsToSkip` and some extra
> traversals. I believe it would be worth getting rid of the additional state
> management, but up to you.
I like the idea and actually tried to implement it before, but failed. The
nested-name-specifier case is simple and we could actually implement it.
However, the `ElaboratedTypeLoc` case is more complicated: we have to call
`Base::TraverseTypeLoc` to make sure we recursively visit children of
`ElaboratedTypeLoc::getNamedType` (e.g. template arguments inside the typeloc).
However, `Base::TraverseTypeLoc` automatically calls `VisitTypeLoc` and our
qualifier is forgotten again.
Let me know if there's a way to implement this that I missed, though.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:577
+ /// (!) For the purposes of this function declarations are not considered to
+ /// be references. However, declarations can have:wa references inside
+ /// them, e.g. 'namespace foo = std' references namespace 'std' and this
----------------
kadircet wrote:
> `:wa` is still around `have:wa references` :D
Thanks. This slipped my attention last time :-)
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:609
+ // FIXME: should this be done by 'explicitReference'?
+ if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
+ return;
----------------
kadircet wrote:
> I can see the second check is for getting rid of references coming from macro
> expansions. But what exactly is the first one for? How can we get an invalid
> nameloc, could you also add it as a comment?
Ah, thanks for spotting this. I don't think we want to filter out macro
references, fixed that and added a test
The first one is a precaution against various implicit nodes getting into the
traversal. Normally this shouldn't happen, unless:
- RecursiveASTVisitor has bugs and visits implicit nodes (e.g. generated
begin() calls for range-based-for )
- Users start traversal from an implicitly-generated statement.
I couldn't come up with examples of that actually happening, but I'm also
scared of adding an assertion as I feel this would inevitably fail at some
point.
At the same time, the whole point of this function is to only expose those
references that could be meaningfully found in the source code (e.g. for
refactoring and syntax highlighting), i.e. they should always have (at least)
source location of the name.
Added a comment, let me know if this could be made clearer, though.
================
Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+
----------------
kadircet wrote:
> Is this for testing purposes? maybe move it into `findtargettests.cpp` or
> make it a member helper like `Print(SourceManager&)` so that it can also
> print locations etc.?
Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we risk
ODR violations in the future.
In any case, it's useful to have debug output and `operator<<` is common for
that purpose: it enables `llvm::toString` and, consecutively,
`llvm::formatv("{0}", R)`.
What are the downsides of having it?
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+ std::string AnnotatedCode;
+ unsigned NextOffset = 0;
+ for (unsigned I = 0; I < Refs.size(); ++I) {
----------------
kadircet wrote:
> this sounds more like `LastOffset` or `PrevOffset`
That's the next character in `Code` we need to process, renamed to
`NextCodeChar` to specifically mention what string we're referring to.
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:504
+ assert(Pos.isValid());
+ if (Pos.isMacroID()) // FIXME: figure out how to show macro locations.
+ Pos = SM.getExpansionLoc(Pos);
----------------
kadircet wrote:
> I believe these are already filtered-out by `findExplicitReferences` ?
Not anymore (wasn't supposed to in the first place). Added tests too.
See the other comment for details.
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:600
+ "5: targets = {a::b::S}\n"
+ "6: targets = {a::b::S::type}, qualifier = 'struct S::'\n"},
+ // Simple templates.
----------------
kadircet wrote:
> Is it OK for this case to be different than `X::a` above?
>
> also shouldn't this be `a::b::struct S` or `None` ?(my preference would be
> towards None)
Qualifier captures exactly what's written in the source code, so it seems
correct: `S::` is what written down.
`struct` comes from the printing function in clang and I don't think it's worth
changing. I believe this comes from the fact that qualifier is a type in this
case.
Why would the qualifier be `None`? Maybe the purpose of this field is unclear?
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+ "0: targets = {y}\n"
+ "1: targets = {Y::foo}\n"},
+ };
----------------
kadircet wrote:
> again qualifiers seems to be inconsistent with the rest of the cases.
There are no qualifiers written in the source code in any of the two
references, therefore both qualifiers are empty.
Again, please let me know if the purpose of `Qualifier` is unclear. Happy to
comment or change to make its purpose clear.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67826/new/
https://reviews.llvm.org/D67826
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
