sammccall marked 4 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:323
+ /// Describe the AST subtree for a piece of code.
+ void getAST(PathRef File, Range, Callback<llvm::Optional<ASTNode>>);
+
----------------
adamcz wrote:
> Any reason not to name arguments here?
I generally prefer not to name them in the declaration when it doesn't
communicate anything beyond the type.
Done here as most of the other functions name them.
(I'm curious, do you think there are useful names here, or is the irregularity
between named/unnamed bothersome?)
================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:241
+ return MTE->isBoundToLvalueReference() ? "lvalue" : "rvalue";
+ return "";
+ }
----------------
adamcz wrote:
> Maybe instead of "" the fallback should be to just return the code in the
> getSourceRange() range?
This goes against a couple of related goals I have here:
- the detail should be short enough to quickly scan, roughly it should be one
name (a class name is OK, but an arbitrary function signature isn't)
- the detail should relate to this *node*, not the whole subtree
================
Comment at: clang-tools-extra/clangd/Protocol.h:1583
+ /// The text document.
+ TextDocumentIdentifier textDocument;
+
----------------
adamcz wrote:
> We should really disable readability-identifier-naming check on this file,
> it's complaining on every single identifier.
Agree - but I don't actually know how to do this without turning it off for the
whole directory. (We could do that...)
================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104
+ };
+ for (const auto &Case : Cases) {
+ ParsedAST AST = TestTU::withCode(Case.first).build();
----------------
adamcz wrote:
> Can you add a test case for class with overloaded operator(s)? They tend to
> cause issues here and there.
Added one with use of an overloaded operator (LMK if you wanted to dump the
declaration instead)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89571/new/
https://reviews.llvm.org/D89571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
