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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks good!

- the plumbing can be simplified by moving the option into ParseOptions, sorry about the churn!
- we need to add some tests, probably easiest by checking for the diagnostics that are now produced. In `unittests/DiagnosticsTests.cpp` there's `TEST(DiagnosticTest, MakeUnique)` which checks the existing make_unique support in this way. We should have a similar function with a different name and verify that it does/doesn't produce diags depending on the flag. We should probably also have a check that two levels of forwarding works.



================
Comment at: clang-tools-extra/clangd/Preamble.cpp:150
+          // std::make_unique is trivial, and we diagnose bad constructor calls.
+          if (FT->isInStdNamespace() && II->isStr("make_unique"))
+            return false;
----------------
nit: reverse the order of the && here, the isStr should be cheaper to check


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:155
+      }
+      // Slow path: Check whether its last parameter is a parameter pack...
+      if (const auto *FD = FT->getAsFunction()) {
----------------
rather than inlining all this here, can you pull out a function `isLikelyForwardingFunction(FunctionTemplateDecl*)`?
That documents the high-level goal and moves that logic away from the flow control of the different types of decls/options that shouldSkipFunctionBody deals with. It also avoids the confusing "return false" when you found what you were looking for!


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:155
+      }
+      // Slow path: Check whether its last parameter is a parameter pack...
+      if (const auto *FD = FT->getAsFunction()) {
----------------
sammccall wrote:
> rather than inlining all this here, can you pull out a function `isLikelyForwardingFunction(FunctionTemplateDecl*)`?
> That documents the high-level goal and moves that logic away from the flow control of the different types of decls/options that shouldSkipFunctionBody deals with. It also avoids the confusing "return false" when you found what you were looking for!
somewhere we need to explain *why* we're parsing these functions. Something like:

```Usually we don't need to look inside the bodies of header functions to understand the program. However when forwarding function like emplace() forward their arguments to some other function, the 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 instantiate) the bodies.```

This could either be here, or possibly on the option inside ParseOptions.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:156
+      // Slow path: Check whether its last parameter is a parameter pack...
+      if (const auto *FD = FT->getAsFunction()) {
+        const auto NumParams = FD->getNumParams();
----------------
nit: getTemplatedDecl().

(getAsFunction() is a loose do-what-I-mean helper, and you know the precise case you're dealing with here)

There's no need for a null check here, you can't have a FunctionTemplateDecl with no function.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:160
+          const auto *LastParam = FD->getParamDecl(NumParams - 1);
+          if (isa<PackExpansionType>(LastParam->getType())) {
+            const auto *PackTypePtr =
----------------
nit: if (const auto* PET = dyn_cast<PackExpansionType>(LastParam->getType()))
and then use it below


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:163
+                dyn_cast<TemplateTypeParmType>(LastParam->getType()
+                                                   .getNonPackExpansionType()
+                                                   .getNonReferenceType()
----------------
again, this is a loose function where you can use a precise one.
PET->getPattern()


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:167
+            // ... whose template parameter comes from the function directly
+            if (FT->getTemplateParameters()->getDepth() ==
+                PackTypePtr->getDepth()) {
----------------
upsj wrote:
> Can PackTypePtr ever be nullptr?
sure: `template <class...T> int foo(T*...)`.
or even some other more exotic derived type.

I don't think we want to include that case, so a null check is fine, with another comment: `// ... of the form T&&...`


================
Comment at: clang-tools-extra/clangd/Preamble.h:103
               const ParseInputs &Inputs, bool StoreInMemory,
+              bool ParseForwardingFunctions,
               PreambleParsedCallback PreambleCallback,
----------------
instead of passing this as a separate parameter, if you make it part of the ParseOptions then it's available in ParseInputs.
This reduces the amount of plumbing/noise needed.


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:208
+
+    // If true, parse make_unique-like functions in the preamble.
+    bool PreambleParseForwardingFunctions = false;
----------------
can you change "make_unique" to "emplace"?
It's confusing if our example is the one function that gets parsed either way.


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:209
+    // If true, parse make_unique-like functions in the preamble.
+    bool PreambleParseForwardingFunctions = false;
   };
----------------
TUScheduler should not need to know about this option at all, it can be passed from ClangdServer through to buildPreamble by placing it in the ParseInputs, specifically in the ParseOptions struct.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:502
+opt<bool> PreambleParseForwardingFunctions{
+    "preamble-parse-forwarding", cat(Misc),
+    desc("Parse all make_unique-like functions in the preamble"), Hidden,
----------------
Preamble is a bit jargony for a command-line flag. And "forwarding" is dangling.
Maybe "parse-forwarding-functions"?
(Internally the name is fine though)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:503
+    "preamble-parse-forwarding", cat(Misc),
+    desc("Parse all make_unique-like functions in the preamble"), Hidden,
+    init(ClangdServer::Options().PreambleParseForwardingFunctions)};
----------------
again, make_unique -> emplace


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:504
+    desc("Parse all make_unique-like functions in the preamble"), Hidden,
+    init(ClangdServer::Options().PreambleParseForwardingFunctions)};
+
----------------
nit: use a trailing comma to get consistent formatting with the others (UseDirtyHeaders is an anomaly)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124688/new/

https://reviews.llvm.org/D124688