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

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG71cb8c8cb9c1: [clangd] parse all make_unique-like functions 
in preamble (authored by upsj, committed by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -66,6 +66,9 @@
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
+  // Parse options pass on to the ParseInputs
+  ParseOptions ParseOpts = {};
+
   // Whether to use overlay the TestFS over the real filesystem. This is
   // required for use of implicit modules.where the module file is written to
   // disk and later read back.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -109,6 +109,7 @@
 ParsedAST TestTU::build() const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = ParseOpts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -389,7 +389,7 @@
 namespace std {
 // These mocks aren't quite right - we omit unique_ptr for simplicity.
 // forward is included to show its body is not needed to get the diagnostic.
-template  T&& forward(T& t) { return static_cast(t); }
+template  T&& forward(T& t);
 template  T* make_unique(A&&... args) {
return new T(std::forward(args)...);
 }
@@ -402,6 +402,32 @@
"no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is only parsed when --parse-forwarding-functions is set
+  Annotations Main(R"cpp(
+struct S { S(char*); };
+auto x = std::[[make_shared]](42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+namespace std {
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the diagnostic.
+template  T&& forward(T& t);
+template  T* make_shared(A&&... args) {
+   return new T(std::forward(args)...);
+}
+}
+  )cpp";
+  TU.ParseOpts.PreambleParseForwardingFunctions = true;
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "in template: "
+   "no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
 template  struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -497,6 +497,14 @@
   Hidden,
   init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt PreambleParseForwardingFunctions{
+"parse-forwarding-functions",
+cat(Misc),
+desc("Parse all emplace-like functions in included headers"),
+Hidden,
+init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt EnableMallocTrim{
 "malloc-trim",
@@ -934,6 +942,7 @@
 Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak ) {
 if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

@sammccall Feel free to merge with Author: Tobias Ribizel 
Thanks for the vote of confidence, I'll apply once the forwarding stuff is done 
:)


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 429653.
upsj marked 4 inline comments as done.
upsj added a comment.

review updates


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -66,6 +66,9 @@
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
+  // Parse options pass on to the ParseInputs
+  ParseOptions ParseOpts = {};
+
   // Whether to use overlay the TestFS over the real filesystem. This is
   // required for use of implicit modules.where the module file is written to
   // disk and later read back.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -107,6 +107,7 @@
 ParsedAST TestTU::build() const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = ParseOpts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -389,7 +389,7 @@
 namespace std {
 // These mocks aren't quite right - we omit unique_ptr for simplicity.
 // forward is included to show its body is not needed to get the diagnostic.
-template  T&& forward(T& t) { return static_cast(t); }
+template  T&& forward(T& t);
 template  T* make_unique(A&&... args) {
return new T(std::forward(args)...);
 }
@@ -402,6 +402,32 @@
"no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is only parsed when --parse-forwarding-functions is set
+  Annotations Main(R"cpp(
+struct S { S(char*); };
+auto x = std::[[make_shared]](42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+namespace std {
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the diagnostic.
+template  T&& forward(T& t);
+template  T* make_shared(A&&... args) {
+   return new T(std::forward(args)...);
+}
+}
+  )cpp";
+  TU.ParseOpts.PreambleParseForwardingFunctions = true;
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "in template: "
+   "no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
 template  struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -498,6 +498,14 @@
   Hidden,
   init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt PreambleParseForwardingFunctions{
+"parse-forwarding-functions",
+cat(Misc),
+desc("Parse all emplace-like functions in included headers"),
+Hidden,
+init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt EnableMallocTrim{
 "malloc-trim",
@@ -935,6 +943,7 @@
 Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak ) {
 if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -126,6 +126,8 @@
 Inputs.CompileCommand = Cmd;
 Inputs.TFS = 
 Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+

[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 
 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

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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:416
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the 
diagnostic.
+template  T&& forward(T& t) { return static_cast(t); }

nridge wrote:
> I'm confused about this line: you say "show its body is not needed" but 
> forward has a body here
Good catch, that was copy-pasted from std::make_unique above, but it doesn't 
seem to be necessary there either. I'll remove the bodies both.


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 429630.
upsj marked an inline comment as done.
upsj added a comment.

remove std::forward bodies from diagnostic tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -81,7 +81,7 @@
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
-  ParsedAST build() const;
+  ParsedAST build(ParseOptions Opts = {}) const;
   std::shared_ptr
   preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -104,9 +104,10 @@
   /*StoreInMemory=*/true, PreambleCallback);
 }
 
-ParsedAST TestTU::build() const {
+ParsedAST TestTU::build(ParseOptions Opts) const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = Opts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -389,7 +389,7 @@
 namespace std {
 // These mocks aren't quite right - we omit unique_ptr for simplicity.
 // forward is included to show its body is not needed to get the diagnostic.
-template  T&& forward(T& t) { return static_cast(t); }
+template  T&& forward(T& t);
 template  T* make_unique(A&&... args) {
return new T(std::forward(args)...);
 }
@@ -402,6 +402,31 @@
"no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is only parsed when --parse-forwarding-functions is set
+  Annotations Main(R"cpp(
+struct S { S(char*); };
+auto x = std::[[make_shared]](42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+namespace std {
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the diagnostic.
+template  T&& forward(T& t);
+template  T* make_shared(A&&... args) {
+   return new T(std::forward(args)...);
+}
+}
+  )cpp";
+  EXPECT_THAT(
+  *TU.build({/*PreambleParseForwardingFunctions=*/true}).getDiagnostics(),
+  UnorderedElementsAre(Diag(
+  Main.range(), "in template: "
+"no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
 template  struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -498,6 +498,14 @@
   Hidden,
   init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt PreambleParseForwardingFunctions{
+"parse-forwarding-functions",
+cat(Misc),
+desc("Parse all emplace-like functions in included headers"),
+Hidden,
+init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt EnableMallocTrim{
 "malloc-trim",
@@ -935,6 +943,7 @@
 Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak ) {
 if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ 

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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I looked through this patch, and it looks good to me! I will leave the final 
approval to Sam though.




Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:416
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the 
diagnostic.
+template  T&& forward(T& t) { return static_cast(t); }

I'm confused about this line: you say "show its body is not needed" but forward 
has a body here


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

@sammccall @nridge can you give this another look? :)


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427911.
upsj added a comment.

update test docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -81,7 +81,7 @@
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
-  ParsedAST build() const;
+  ParsedAST build(ParseOptions Opts = {}) const;
   std::shared_ptr
   preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -104,9 +104,10 @@
   /*StoreInMemory=*/true, PreambleCallback);
 }
 
-ParsedAST TestTU::build() const {
+ParsedAST TestTU::build(ParseOptions Opts) const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = Opts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -402,6 +402,31 @@
"no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is only parsed when --parse-forwarding-functions is set
+  Annotations Main(R"cpp(
+struct S { S(char*); };
+auto x = std::[[make_shared]](42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+namespace std {
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the diagnostic.
+template  T&& forward(T& t) { return static_cast(t); }
+template  T* make_shared(A&&... args) {
+   return new T(std::forward(args)...);
+}
+}
+  )cpp";
+  EXPECT_THAT(
+  *TU.build({/*PreambleParseForwardingFunctions=*/true}).getDiagnostics(),
+  UnorderedElementsAre(Diag(
+  Main.range(), "in template: "
+"no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
 template  struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -498,6 +498,14 @@
   Hidden,
   init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt PreambleParseForwardingFunctions{
+"parse-forwarding-functions",
+cat(Misc),
+desc("Parse all emplace-like functions in included headers"),
+Hidden,
+init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt EnableMallocTrim{
 "malloc-trim",
@@ -935,6 +943,7 @@
 Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak ) {
 if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -126,6 +126,8 @@
 Inputs.CompileCommand = Cmd;
 Inputs.TFS = 
 Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+Inputs.Opts.PreambleParseForwardingFunctions =
+Opts.PreambleParseForwardingFunctions;
 if (Contents.hasValue()) {
   Inputs.Contents = *Contents;
   log("Imaginary source file contents:\n{0}", Inputs.Contents);
Index: clang-tools-extra/clangd/Preamble.cpp

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427910.
upsj marked 13 inline comments as done.
upsj added a comment.

review updates:

- Pass ParseForwardingFunctions via ParseOptions
- Simplify check for forwarding function
- Add test checking diagnostics for make_shared


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -81,7 +81,7 @@
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
-  ParsedAST build() const;
+  ParsedAST build(ParseOptions Opts = {}) const;
   std::shared_ptr
   preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -104,9 +104,10 @@
   /*StoreInMemory=*/true, PreambleCallback);
 }
 
-ParsedAST TestTU::build() const {
+ParsedAST TestTU::build(ParseOptions Opts) const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = Opts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -402,6 +402,31 @@
"no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is an exception.
+  Annotations Main(R"cpp(
+struct S { S(char*); };
+auto x = std::[[make_shared]](42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+namespace std {
+// These mocks aren't quite right - we omit shared_ptr for simplicity.
+// forward is included to show its body is not needed to get the diagnostic.
+template  T&& forward(T& t) { return static_cast(t); }
+template  T* make_shared(A&&... args) {
+   return new T(std::forward(args)...);
+}
+}
+  )cpp";
+  EXPECT_THAT(
+  *TU.build({/*PreambleParseForwardingFunctions=*/true}).getDiagnostics(),
+  UnorderedElementsAre(Diag(
+  Main.range(), "in template: "
+"no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
 template  struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -498,6 +498,14 @@
   Hidden,
   init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt PreambleParseForwardingFunctions{
+"parse-forwarding-functions",
+cat(Misc),
+desc("Parse all emplace-like functions in included headers"),
+Hidden,
+init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt EnableMallocTrim{
 "malloc-trim",
@@ -935,6 +943,7 @@
 Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak ) {
 if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -126,6 +126,8 @@
 Inputs.CompileCommand = Cmd;
 Inputs.TFS = 
 Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+Inputs.Opts.PreambleParseForwardingFunctions =
+Opts.PreambleParseForwardingFunctions;
 if (Contents.hasValue()) {
   

[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(LastParam->getType())) {
+const auto *PackTypePtr =

nit: if (const auto* PET = dyn_cast(LastParam->getType()))
and then use it below



Comment at: clang-tools-extra/clangd/Preamble.cpp:163
+dyn_cast(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  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 , 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 

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427854.
upsj added a comment.

accidentally pushed to the wrong revision, this is the previous version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -101,7 +101,9 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  /*StoreInMemory=*/true, PreambleCallback);
+  /*StoreInMemory=*/true,
+  /*ParseForwardingFunctions=*/false,
+  PreambleCallback);
 }
 
 ParsedAST TestTU::build() const {
@@ -115,9 +117,10 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
 
-  auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-   /*StoreInMemory=*/true,
-   /*PreambleCallback=*/nullptr);
+  auto Preamble =
+  clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
+   /*StoreInMemory=*/true,
+   /*PreambleCallback=*/false, nullptr);
   auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
   Diags.take(), Preamble);
   if (!AST.hasValue()) {
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -173,7 +173,8 @@
   TU.AdditionalFiles["c.h"] = "";
   auto PI = TU.inputs(FS);
   auto BaselinePreamble = buildPreamble(
-  TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, nullptr);
+  TU.Filename, *buildCompilerInvocation(PI, Diags), PI,
+  /*StoreInMemory=*/true, /*ParseForwardingFunctions=*/false, nullptr);
   // We drop c.h from modified and add a new header. Since the latter is patched
   // we should only get a.h in preamble includes.
   TU.Code = R"cpp(
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -497,7 +497,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto EmptyPreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(EmptyPreamble);
   EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
@@ -540,7 +541,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto BaselinePreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(BaselinePreamble);
   EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes,
   ElementsAre(testing::Field(::Written, "")));
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -306,7 +306,7 @@
   FileIndex Index;
   bool IndexUpdated = false;
   buildPreamble(FooCpp, *CI, PI,
-/*StoreInMemory=*/true,
+/*StoreInMemory=*/true, /*ParseForwardingFunctions=*/false,
 [&](ASTContext , Preprocessor ,
 const CanonicalIncludes ) {
   EXPECT_FALSE(IndexUpdated)
Index: 

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 427852.
upsj added a comment.

Work around the currently broken isExpandedParameter, also fix the broken diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -170,6 +170,43 @@
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+template 
+void foo(Args&& ...);
+void bar() {
+  foo(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // The forward prototype is not correct, but is converted into builtin anyways
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int);
+template 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar(42);
+}
+  )cpp");
+}
+
 TEST(ParameterHints, NameInDefinition) {
   // Parameter name picked up from definition if necessary.
   assertParameterHints(R"cpp(
@@ -254,6 +291,123 @@
ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter
+  // The forward prototype is not correct, but is converted into builtin anyways
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T bar(Args&&... args) { return T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter
+  // The forward prototype is not correct, but is converted into builtin anyways
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{std::forward(args)...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+struct S { S(int a); };
+template 
+T* bar(Args&&... args) { return new T{args...}; }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter
+  // The forward prototype is not correct, but is converted into builtin anyways
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  int b;
+  bar($param[[b]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(args...); }
+void baz() {
+  bar($param[[42]]);
+}
+  )cpp",
+   ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, MatchingNameVariadicForwarded) {
+  // No name hint for variadic parameter with matching name
+  // The forward prototype is not correct, but is converted into builtin anyways
+  assertParameterHints(R"cpp(
+namespace std { template  T&& forward(T&); }
+void foo(int a);
+template 
+void bar(Args&&... args) { return foo(std::forward(args)...); }
+void baz() {
+  int a;
+  bar(a);
+}
+  )cpp");
+}
+
+TEST(ParameterHints, MatchingNameVariadicPlain) {
+  // No name hint for variadic parameter with matching name
+  assertParameterHints(R"cpp(
+void foo(int a);
+template 
+void bar(Args&&... args) { return 

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:167
+// ... whose template parameter comes from the function directly
+if (FT->getTemplateParameters()->getDepth() ==
+PackTypePtr->getDepth()) {

Can PackTypePtr ever be nullptr?


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj updated this revision to Diff 426387.
upsj marked 2 inline comments as done.
upsj added a comment.
Herald added a subscriber: javed.absar.

Rewrote detection of forwarding functions in preamble, added 
`--preamble-parse-forwarding` flag

I ran clangd over  `bits/stdc++.h`, and it seems like the condition works very 
well:

It picked up

- container::emplace and friends
- allocator::construct and friends
- make_unique, make_shared and friends
- some parts of `functional`
- some parts of `tuple`
- some parts of `thread`

most of which seem like good candidates for forwarding parameters.

In terms of runtime, I can't see any noticeable difference between parsing with 
or without `--preamble-parse-forwarding`, but of course, all of libstdc++ is 
not the most realistic scenario


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -101,7 +101,9 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  /*StoreInMemory=*/true, PreambleCallback);
+  /*StoreInMemory=*/true,
+  /*ParseForwardingFunctions=*/false,
+  PreambleCallback);
 }
 
 ParsedAST TestTU::build() const {
@@ -115,9 +117,10 @@
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
 
-  auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-   /*StoreInMemory=*/true,
-   /*PreambleCallback=*/nullptr);
+  auto Preamble =
+  clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
+   /*StoreInMemory=*/true,
+   /*PreambleCallback=*/false, nullptr);
   auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
   Diags.take(), Preamble);
   if (!AST.hasValue()) {
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -173,7 +173,8 @@
   TU.AdditionalFiles["c.h"] = "";
   auto PI = TU.inputs(FS);
   auto BaselinePreamble = buildPreamble(
-  TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, nullptr);
+  TU.Filename, *buildCompilerInvocation(PI, Diags), PI,
+  /*StoreInMemory=*/true, /*ParseForwardingFunctions=*/false, nullptr);
   // We drop c.h from modified and add a new header. Since the latter is patched
   // we should only get a.h in preamble includes.
   TU.Code = R"cpp(
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -497,7 +497,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto EmptyPreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(EmptyPreamble);
   EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
@@ -540,7 +541,8 @@
   auto Inputs = TU.inputs(FS);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto BaselinePreamble =
-  buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
+  buildPreamble(testPath("foo.cpp"), *CI, Inputs, /*StoreInMemory=*/true,
+/*PreambleCallback=*/false, nullptr);
   ASSERT_TRUE(BaselinePreamble);
   EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes,
   ElementsAre(testing::Field(::Written, "")));

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D124688#3485042 , @nridge wrote:

> In D124688#3483895 , @sammccall 
> wrote:
>
>> Trying this on a few files, this seems like it increases preamble sizes up 
>> to 1% or so
>
> Are preamble sizes a good proxy for preamble build times as well? I suspect 
> that may be the metric that's more noticeable for users.

Agree, yes I **think** they're a pretty good proxy for this sort of change 
which parses more/less code. (Preamble size more or less counting AST nodes).


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

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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D124688#3483895 , @sammccall wrote:

> Trying this on a few files, this seems like it increases preamble sizes up to 
> 1% or so

Are preamble sizes a good proxy for preamble build times as well? I suspect 
that may be the metric that's more noticeable for users.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Great! If this were to be a public, user-controlled feature we'd make it part 
of the config file  which is a bit more 
involved, but I think this is rather just a developer toggle until it's time to 
turn it on by default, so a command-line flag seems fine.

This needs to be passed from the main binary => ClangdServer => 
Preamble::build, as something like `bool AlwaysParseForwardingFunctions`.

I think the most appropriate place to pass this to Preamble::build is in 
ParseOptions defined in `Compiler.h` (currently empty) which is embedded in 
ParseInputs. Something like `bool AlwaysParseForwardingFunctions`.
And the usual way to pass command line flags into ClangdServer is via 
ClangdServer::Options.

See 
https://github.com/llvm/llvm-project/commit/e6be5c7cd6d227144f874623e2764890f80cad32
 where we removed a couple of ParseOptions, I think you'd basically want the 
opposite of that.
(Sorry about the plumbing, configuration is always a pain).

For testing, you want the ability to turn this option on in TestTU. I'd just 
add `ParseOptions TestTU::ParseOpts` as a public member that your test can set, 
and use it from `TestTU::inputs()` instead of the current `Inputs.Opts = 
ParseOptions();`.


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

Thanks for checking! Putting it behind a flag was my intention from the start 
anyways, since ideally I would like to traverse the AST through something like 
emplace_back ->construct/realloc_insert -> allocator::construct until I reach a 
non-forwarding function.
Do you have some pointers on what needs to be done to add a new flag? I am 
still kind of new to LLVM development :)


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Trying this on a few files, this seems like it increases preamble sizes up to 
1% or so:

   | Before| After
  -+---+--
  AST.cpp  | 42249648  | 42419732
  XRefsTests.cpp   | 56525116  | 56763768
  SelectionDAGISel.cpp | 37546764  | 37668564
  internal_s.cc| 59582216  | 60024876
  internal_m.cc| 192084984 | 193850560
  internal_l.cc| 365811816 | 368841388

I can't see any reason to think that RAM/CPU usage would be out of proportion 
to this.
A 1% regression here isn't trivial but seems worthwhile for significant 
functional improvements.
I suppose we can add:

- diagnostics (already with this patch)
- inlay hints
- signature help

WDYT about keeping this behind a flag until these are working?
For diagnostics alone, I'm not sure this is a good tradeoff (otherwise why 
would we restrict it to variadics? FWIW allowing all function templates to be 
parsed is +4% to preamble size for internal_l.cc.

I'm less concerned about the effects on the main file, partly because preambles 
are a bigger performance cliff, and partly because we're probably only 
regressing in cases where the user really is seeing benefits too.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: adamcz.
sammccall added a comment.

It was intentional to only (initially) support std::make_unique as its 
implementation is trivial. Whereas I looked at std::make_shared and it appears 
to result in instantiating a lot of templates.
This was more "I'm sure this is useful and cheap" than "I'm sure covering more 
functions is too expensive" though.

I suspect that parsing all forwarding function bodies is quite expensive (well, 
parsing them => instantiating them => instantiating everything from their 
bodies).
We can measure this in a couple of ways:

- some work will happen while parsing headers, this will show up as increased 
preamble size which is easy to measure reliably
- some will only happen when parsing main files, this will cause increased RAM 
and latency in main-file parses, which is less reproducible

I'll try patching this and at least see what happens to preamble sizes on some 
big files

@adamcz FYI (Adam was looking at forwarding functions, for completion/signature 
help, but isn't anymore I believe)




Comment at: clang-tools-extra/clangd/Preamble.cpp:144
+  // ... with a template parameter pack...
+  if (FT->getTemplateParameters()->hasParameterPack()) {
+auto PackIt = std::find_if(

this is a linear search, and so is the next line, let's just do it once :-)



Comment at: clang-tools-extra/clangd/Preamble.cpp:144
+  // ... with a template parameter pack...
+  if (FT->getTemplateParameters()->hasParameterPack()) {
+auto PackIt = std::find_if(

sammccall wrote:
> this is a linear search, and so is the next line, let's just do it once :-)
I feel that checking template params -> injected template args -> params is a 
bit roundabout. (And it involves some searching and construction of the 
injected template args).

Possibly more direct, and I think equivalent:
- get the last function param, check that it's a pack and unwrap
- check that its (non-ref) type is a TemplateTypeParmType
- verify that the param is from the function template rather than elsewhere, by 
comparing type->getDepth() == funcTemplate->getTemplateParameters()->getDepth()
I think all these are constant-time, too


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

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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added reviewers: sammccall, kadircet.
nridge added a comment.

I'll have a proper look at this when I get a chance but meanwhile adding Sam 
and Kadir who may have thoughts


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

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

[Tobias Ribizel via Phabricator via cfe-commits]

upsj created this revision.
upsj added a reviewer: nridge.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
upsj requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

I am working on support for forwarding parameter names in make_unique-like 
functions, first for inlay hints, later maybe for signature help.
For that to work generically, I'd like to parse all of these functions in the 
preamble. Not sure how this impacts performance on large codebases though.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124688

Files:
  clang-tools-extra/clangd/Preamble.cpp


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -137,14 +137,42 @@
   }
 
   bool shouldSkipFunctionBody(Decl *D) override {
-// Generally we skip function bodies in preambles for speed.
-// We can make exceptions for functions that are cheap to parse and
-// instantiate, widely used, and valuable (e.g. commonly produce errors).
-if (const auto *FT = llvm::dyn_cast(D)) {
-  if (const auto *II = FT->getDeclName().getAsIdentifierInfo())
-// std::make_unique is trivial, and we diagnose bad constructor calls.
-if (II->isStr("make_unique") && FT->isInStdNamespace())
-  return false;
+// Find functions with variadic template arguments:
+// Any templated function...
+if (auto *FT = llvm::dyn_cast(D)) {
+  // ... with a template parameter pack...
+  if (FT->getTemplateParameters()->hasParameterPack()) {
+auto PackIt = std::find_if(
+FT->getInjectedTemplateArgs().begin(),
+FT->getInjectedTemplateArgs().end(), [](const auto ) {
+  return Arg.getKind() == TemplateArgument::Pack;
+});
+assert(PackIt != FT->getInjectedTemplateArgs().end() &&
+   "Can't find parameter pack in argument list!");
+const auto  = PackIt->getPackAsArray();
+
+// ... that is a type parameter pack...
+if (Pack.size() == 1 && Pack[0].getKind() == TemplateArgument::Type) {
+  const auto *PackType =
+  Pack[0].getAsType().getNonPackExpansionType().getTypePtr();
+  const auto *FD = FT->getAsFunction();
+  const auto NumParams = FD->getNumParams();
+  if (NumParams > 0) {
+const auto *LastParam = FD->getParamDecl(NumParams - 1);
+// ... with its type matching the last parameter (pack) of the
+// function (minus references)...
+if (LastParam->isParameterPack()) {
+  if (LastParam->getType()
+  .getNonPackExpansionType()
+  .getNonReferenceType()
+  .getTypePtr() == PackType) {
+// ... we need to parse the body
+return false;
+  }
+}
+  }
+}
+  }
 }
 return true;
   }


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -137,14 +137,42 @@
   }
 
   bool shouldSkipFunctionBody(Decl *D) override {
-// Generally we skip function bodies in preambles for speed.
-// We can make exceptions for functions that are cheap to parse and
-// instantiate, widely used, and valuable (e.g. commonly produce errors).
-if (const auto *FT = llvm::dyn_cast(D)) {
-  if (const auto *II = FT->getDeclName().getAsIdentifierInfo())
-// std::make_unique is trivial, and we diagnose bad constructor calls.
-if (II->isStr("make_unique") && FT->isInStdNamespace())
-  return false;
+// Find functions with variadic template arguments:
+// Any templated function...
+if (auto *FT = llvm::dyn_cast(D)) {
+  // ... with a template parameter pack...
+  if (FT->getTemplateParameters()->hasParameterPack()) {
+auto PackIt = std::find_if(
+FT->getInjectedTemplateArgs().begin(),
+FT->getInjectedTemplateArgs().end(), [](const auto ) {
+  return Arg.getKind() == TemplateArgument::Pack;
+});
+assert(PackIt != FT->getInjectedTemplateArgs().end() &&
+   "Can't find parameter pack in argument list!");
+const auto  = PackIt->getPackAsArray();
+
+// ... that is a type parameter pack...
+if (Pack.size() == 1 && Pack[0].getKind() == TemplateArgument::Type) {
+  const auto *PackType =
+  Pack[0].getAsType().getNonPackExpansionType().getTypePtr();
+  const auto *FD = FT->getAsFunction();
+  const auto NumParams = FD->getNumParams();
+  if (NumParams > 0) {
+const auto *LastParam =