malaperle-ericsson added inline comments.
================
Comment at: clangd/ClangdUnit.cpp:303
+ // fo|o -> foo| good!
+ if (InputLocation == SourceLocationBeg && Pos.character > 0) {
+ SourceLocation PeekBeforeLocation = Unit->getLocation(FE, Pos.line + 1,
----------------
ilya-biryukov wrote:
> Minor: Maybe invert condition to reduce nesting, i.e. if (InputLocation !=
> SourceLocationBeg || Pos.character == 0) return SourceLocationBeg;
>
> PS. I'm perfectly fine if this comment is ignored, it's just a matter of
> preference.
I like it better with the return
================
Comment at: clangd/ClangdUnit.cpp:306
+ Pos.character);
+ SourceLocation PeekBeforeLocationEnd = Lexer::getLocForEndOfToken(
+ PeekBeforeLocation, 0, SourceMgr, Unit->getASTContext().getLangOpts());
----------------
ilya-biryukov wrote:
> Just wondering: is there a way to not do the lexing multiple times?
I was able to simplify this a bit. There's on only one call to getRawToken and
one to GetBeginningOfToken.
================
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) {
----------------
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.
================
Comment at: clangd/DeclarationLocationsFinder.cpp:59
+ Token Result;
+ if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(),
+ Unit.getASTContext().getLangOpts(), false)) {
----------------
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.
================
Comment at: clangd/Protocol.cpp:57
std::string URI::unparse(const URI &U) {
- return U.uri;
+ return "\"" + U.uri + "\"";
}
----------------
ilya-biryukov wrote:
> Why this didn't require any changes to other code? This method hasn't been
> used before?
No it wasn't used before.
================
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.
----------------
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!
https://reviews.llvm.org/D34269
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
