ilya-biryukov added a comment.
> Another note: current implementation does not seem to handle macros at all
> (it will only highlight definition of a macro, not its usages).
Current implementation still doesn't highlight macro references, only
definitions. I'd either disable `documentHighlight` for macros for the initial
version or come up with a way to implement `documentHighlight` for them.
I don't think `Preprocessor` currently stores information about all macro
references, so we'll need to either store a separate index for that or rerun
preprocessor to get this information.
================
Comment at: clangd/ClangdUnit.cpp:1017
+
+ auto DeclLocationsFinder = std::make_shared<TargetDeclarationFinder>(
+ llvm::errs(), SourceLocationBeg, AST.getASTContext(),
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > I wonder if we really need to rerun indexer twice here. Probably ok for the
> > first version, but we should definitely think about a faster way to get the
> > `Decl` under cursor than visiting the whole AST. Not sure if it's easy to
> > do with clang's AST, though, maybe we'll need a separate index for that.
> Yeah I realise it's definitely not the fastest way to go about it. Maybe
> there is a way to stop handling occurrences once the highlighted declaration
> is found. This would be the most straightforward way of improving the
> performance of the feature without re-writing a new AST indexer.
That only helps if we get lucky, average case and worst case would still be
slow.
Anyway, this commit can certainly go in without this change.
================
Comment at: clangd/ClangdUnit.cpp:879
public:
+ std::vector<const Decl *> Decls;
+ std::vector<MacroInfo *> MacroInfos;
----------------
Let's not store `DeclarationLocations` anymore. If we have `Decl`s and
`MacroInfo`s, we can move the code getting their `Location`s out of the
`DeclarationLocationsFinder`.
================
Comment at: clangd/ClangdUnit.cpp:880
+ std::vector<const Decl *> Decls;
+ std::vector<MacroInfo *> MacroInfos;
DeclarationLocationsFinder(raw_ostream &OS,
----------------
Please make the fields private, e.g. put them before `SearchedLocations`.
================
Comment at: clangd/ClangdUnit.cpp:976
+public:
+ std::vector<const Decl *> Decls;
+ std::vector<MacroInfo *> MacroInfos;
----------------
Make these fields private.
Don't copy `std::vector`s, store references to them instead.
================
Comment at: clangd/ClangdUnit.cpp:1003
+ const SourceManager &SourceMgr = AST.getSourceManager();
+ if (std::find(Decls.begin(), Decls.end(), D) != Decls.end()) {
+ SourceLocation Begin, End;
----------------
Please rewrite
```
if (cond) {
// long code
}
return true;
```
to
```
if (!cond)
return true;
// long code
```
This reduces nesting and makes code easier to read.
See [[
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
| style guide ]].
================
Comment at: clangd/Protocol.h:431
+ */
+struct DocumentHighlight{
+ /**
----------------
ilya-biryukov wrote:
> There seems to be a missing brace before `{`. I generally run this simple
> script before submission to make sure my code is always formatted:
> ```
> find "$CLANG_TOOLS_EXTRA_PATH/clangd" -name "*.cpp" -or -name "*.h" -exec
> clang-format -i --style=LLVM {} \;
> find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" -name "*.cpp" -or -name "*.h"
> -exec clang-format -i --style=LLVM {} \ ;
> ```
BTW, the script had an error. (missing some parens)
```
find "$CLANG_TOOLS_EXTRA_PATH/clangd" \( -name "*.cpp" -or -name "*.h" \) -exec
clang-format -i --style=LLVM {} \;
find "$CLANG_TOOLS_EXTRA_PATH/unittests/clangd" \( -name "*.cpp" -or -name
"*.h" \) -exec clang-format -i --style=LLVM {} \ ;
```
================
Comment at: test/clangd/initialize-params.test:24
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+
+
----------------
Redundant newlines at the end of the file.
https://reviews.llvm.org/D38425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
