adamcz added inline comments.
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:795 + unsigned Start, End; + if (auto O = positionToOffset(Inputs->Inputs.Contents, R.start)) + Start = *O; ---------------- Please rename O to anything else. *O below looks very much like *0 and it had me really confused. ================ 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>>); + ---------------- Any reason not to name arguments here? ================ Comment at: clang-tools-extra/clangd/DumpAST.cpp:241 + return MTE->isBoundToLvalueReference() ? "lvalue" : "rvalue"; + return ""; + } ---------------- Maybe instead of "" the fallback should be to just return the code in the getSourceRange() range? ================ Comment at: clang-tools-extra/clangd/DumpAST.cpp:393 + // DynTypedNode only works with const, RecursiveASTVisitor only non-const :-( + if (auto *D = N.get<Decl>()) + V.TraverseDecl(const_cast<Decl *>(D)); ---------------- Lots of lint warnings here about "const". Please fix. ================ Comment at: clang-tools-extra/clangd/Protocol.h:1583 + /// The text document. + TextDocumentIdentifier textDocument; + ---------------- We should really disable readability-identifier-naming check on this file, it's complaining on every single identifier. ================ Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104 + }; + for (const auto &Case : Cases) { + ParsedAST AST = TestTU::withCode(Case.first).build(); ---------------- Can you add a test case for class with overloaded operator(s)? They tend to cause issues here and there. 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