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