adamcz accepted this revision. adamcz added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:799 + return CB(Offset.takeError()); + if (auto O = positionToOffset(Inputs->Inputs.Contents, R.end)) + End = *O; ---------------- Also here please ;) ================ 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>>); + ---------------- sammccall wrote: > 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?) Unnamed argument makes me think that it's ignored by this function. It's in-line with Google style guide, maybe doesn't necessary apply here. ================ Comment at: clang-tools-extra/clangd/DumpAST.cpp:209 + std::string getDetail(const Decl *D) { + auto *ND = dyn_cast<NamedDecl>(D); + if (!ND || llvm::isa_and_nonnull<CXXConstructorDecl>(ND->getAsFunction()) || ---------------- lint says this can be const ================ Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:12 +#include "TestTU.h" +#include "llvm/Support/ScopedPrinter.h" +#include "gmock/gmock.h" ---------------- include order lint warning here ================ Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104 + }; + for (const auto &Case : Cases) { + ParsedAST AST = TestTU::withCode(Case.first).build(); ---------------- sammccall wrote: > 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) Thanks, that's exactly what I wanted. 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