[PATCH] D124688: [clangd] parse all make_unique-like functions in preamble

[Sam McCall via Phabricator via cfe-commits]

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