ilya-biryukov added a comment. Hi @malaperle-ericsson. Thanks for the patch. IndexingAction has a very simple interface, exactly what clangd needs and nothing more.
BTW, we had a bug open for that: https://bugs.llvm.org/show_bug.cgi?id=33261, feel free to reassign to yourself. ================ Comment at: clangd/ClangdServer.cpp:275 + auto TaggedFS = FSProvider.getTaggedFileSystem(File); + Units.runOnUnitWithoutReparse( + File, *FileContents.Draft, CDB, PCHs, TaggedFS.Value, [&](ClangdUnit &Unit) { ---------------- We must call **runOnUnit** here, calling **runOnUnitWithoutReparse** will result in an outdated AST. (This can't happen if you use -run-synchronously, so tests don't catch it). ================ Comment at: clangd/ClangdUnit.cpp:303 + // fo|o -> foo| good! + if (InputLocation == SourceLocationBeg && Pos.character > 0) { + SourceLocation PeekBeforeLocation = Unit->getLocation(FE, Pos.line + 1, ---------------- 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. ================ Comment at: clangd/ClangdUnit.cpp:306 + Pos.character); + SourceLocation PeekBeforeLocationEnd = Lexer::getLocForEndOfToken( + PeekBeforeLocation, 0, SourceMgr, Unit->getASTContext().getLangOpts()); ---------------- Just wondering: is there a way to not do the lexing multiple times? ================ 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) { ---------------- 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? ================ 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: > 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() ``` ================ Comment at: clangd/DeclarationLocationsFinder.cpp:59 + Token Result; + if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(), + Unit.getASTContext().getLangOpts(), false)) { ---------------- 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? ================ Comment at: clangd/DeclarationLocationsFinder.h:24 +/// Finds declarations locations that a given source location refers to. +class DeclarationLocationsFinder : public index::IndexDataConsumer { + std::vector<Location> DeclarationLocations; ---------------- Maybe put this into ClangdUnit.cpp inside an anonymous namespace? As it's just an implementation detail. ================ Comment at: clangd/DeclarationLocationsFinder.h:34 + + std::vector<Location> getLocations() { + return DeclarationLocations; ---------------- Maybe have a takeLocations() method that 'std::move's the vector instead of copying? ================ Comment at: clangd/Protocol.cpp:57 std::string URI::unparse(const URI &U) { - return U.uri; + return "\"" + U.uri + "\""; } ---------------- Why this didn't require any changes to other code? This method hasn't been used before? ================ Comment at: clangd/Protocol.h:42 + + friend bool operator==(const URI &LHS, const URI &RHS) { + return LHS.uri == RHS.uri; ---------------- Maybe add 'operator != ' for consistency? ================ Comment at: clangd/Protocol.h:98 + + friend bool operator==(const Location &LHS, const Location &RHS) { + return LHS.uri == RHS.uri && LHS.range == RHS.range; ---------------- Maybe add 'operator != ' for consistency? ================ 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. ---------------- 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. Repository: rL LLVM https://reviews.llvm.org/D34269 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits