sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks, this looks good now! A last few nits here. Would you like me to commit for you? From your patches so far, I think it's appropriate to request commit access <https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access> if you want it. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:221 std::string ActualVersion = DraftMgr.addDraft(File, Version, Contents); - ParseOptions Opts; + ParseOptions Opts{PreambleParseForwardingFunctions}; ---------------- nit: rather assign Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions booleans in these opts structs are particularly prone to accidentally reordering and mis-assigning One day we'll have designated initializers for this... ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:140 + bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) { + const auto *FD = FT->getTemplatedDecl(); ---------------- nit: this can be static, and I think it'd be slightly clearer (e.g. that this doesn't interact with the policy flag) ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:166 + // interesting overload resolution happens inside the forwarding + // function's body. To provide more meaningful meaningful diagnostics, + // code completion, and parameter hints we should parse (and later ---------------- nit: only one "meaningful" :-) ================ Comment at: clang-tools-extra/clangd/unittests/TestTU.h:84 // The result will always have getDiagnostics() populated. - ParsedAST build() const; + ParsedAST build(ParseOptions Opts = {}) const; std::shared_ptr<const PreambleData> ---------------- Instead of adding a parameter here, we should add it to the TestTU struct and use it in inputs(). *many* things affect the behavior of build(), and tests would quickly become unreadable if we passed many of them as parameters. So we hold the line at zero :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124688/new/ https://reviews.llvm.org/D124688 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
