[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 4 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+
----------------
ilya-biryukov wrote:
> 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?
> 
I wasn't happy about it because it actually needs a `SourceManager` to print completely, and I believe that's also necessary for debugging.

But feel free to ignore if you're OK with that.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+    std::string AnnotatedCode;
+    unsigned NextOffset = 0;
+    for (unsigned I = 0; I < Refs.size(); ++I) {
----------------
ilya-biryukov wrote:
> 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.
well, this looked more like "beginning of the last token we processed" to me. but now I see your point of view as well. Feel free to use whichever you want.


================
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.
----------------
ilya-biryukov wrote:
> 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?
ah sorry, I missed the fact that this was written as `S::type` in the code, now it makes sense. just ignore this one.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+           "0: targets = {y}\n"
+           "1: targets = {Y::foo}\n"},
+      };
----------------
ilya-biryukov wrote:
> 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.
> 
yeah it totally makes sense now, the problem started with me missing the `S::` in the previous example :D, nvm..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826