ilya-biryukov added a comment.
Looks good now, but I think we definitely need to change
`unique_ptr<vector<Location>>` to `vector<Location>` before submitting it.
I won't be available until next Wednesday, so feel free to submit without my
final approval.
================
Comment at: clangd/ClangdUnit.cpp:276
+class DeclarationLocationsFinder : public index::IndexDataConsumer {
+ std::unique_ptr<std::vector<Location>> DeclarationLocations;
+ const SourceLocation &SearchedLocation;
----------------
Why do we need to use `unique_ptr<vector<Location>>` here and in other places
instead of `vector<Location>`?
================
Comment at: clangd/DeclarationLocationsFinder.cpp:48
+ // This can happen when nodes in the AST are visited twice.
+ if (std::none_of(DeclarationLocations.begin(), DeclarationLocations.end(),
+ [&L](const Location& Loc) {
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Is it possible for DeclarationLocation to become large, so that quadratic
> > > behavior is observed?
> > >
> > > If not, maybe add an assert that it's smaller than some threshold?
> > > If yes, maybe use std::set instead of std::vector or use vector and later
> > > std::sort and std::unique in the end?
> > Maybe use std::find instead of std::none_of?
> > ```
> > std::find(DeclarationLocations.begin(), DeclarationLocations.end(), L) ==
> > DeclarationLocations.end()
> > ```
> I went with std::sort/std::unique. I don't think this will ever be large but
> I don't think it would be good to have an arbitrary limit either. I think
> keeping the vector and cleaning it later is nice and simple.
Totally agree, nice and simple.
================
Comment at: clangd/DeclarationLocationsFinder.cpp:59
+ Token Result;
+ if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(),
+ Unit.getASTContext().getLangOpts(), false)) {
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > Could we implement ` handleMacroOccurence` instead?
> > I suspect current code won't work if macro is undef'ed/redefined later:
> >
> > ```
> > #define FOO 123
> >
> > int b = FO|O;
> >
> > #define FOO 125
> > // or
> > // #undef FOO
> > ```
> >
> > Maybe also add this to tests?
> You're right! It didn't work properly in this case. I added a few tests.
> For handleMacroOccurence, it actually isn't even called so we'd have to
> improve the clangIndex code to do this. I think this is a good first step
> though.
Awesome!
clangIndex indeed never calls `handleMacroOccurence`, I wonder why it's there
in the first place.
================
Comment at: test/clangd/definitions.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > Could we also add more readable tests specifically for that?
> > I propose to have a tool that could read files like:
> > ```
> > int aaa;
> > int b = a/*{l1}*/aa;
> > int c = /*{l2}*/b;
> > ```
> >
> > And have it output the resulting goto results:
> > ```
> > l1 -> {main.cpp:0:4} int |aaa;
> > l2 -> {main.cpp:1:4} int |b;
> > ```
> > And we could also have a tool that prints expected clangd input/output
> > based on that to check that action actually works.
> > It's not directly relevant to this patch, though. Just random thoughts of
> > what we should do for easier testing.
> I think it's a good idea! Although I wonder if it's a bit too much work for
> something that very specific to "textDocument/definition". I fully agree that
> the tests need to be more readable and it would be worth a try!
We could also reuse most of the code for all caret-based action, so it's
actually useful for other things as well.
https://reviews.llvm.org/D34269
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
