<div dir="ltr">Hi Douglas,<div><br></div><div>Fixed in r315028.</div><div>Sorry for the inconvenience.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 5, 2017 at 11:55 PM, Yung, Douglas <span dir="ltr"><<a href="mailto:douglas.yung@sony.com" target="_blank">douglas.yung@sony.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Ilya,<br>
<br>
Your change has broken the build on the PS4 Windows bot.<br>
<br>
<a href="http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/12525" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/<wbr>builders/llvm-clang-lld-x86_<wbr>64-scei-ps4-windows10pro-fast/<wbr>builds/12525</a><br>
<br>
Can you take a look and fix it?<br>
<br>
Douglas Yung<br>
<div><div class="h5"><br>
> -----Original Message-----<br>
> From: cfe-commits [mailto:<a href="mailto:cfe-commits-bounces@lists.llvm.org">cfe-commits-bounces@<wbr>lists.llvm.org</a>] On Behalf Of<br>
> Ilya Biryukov via cfe-commits<br>
> Sent: Thursday, October 05, 2017 10:04<br>
> To: <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> Subject: [clang-tools-extra] r314989 - [clangd] Added async API to run code<br>
> completion.<br>
><br>
> Author: ibiryukov<br>
> Date: Thu Oct 5 10:04:13 2017<br>
> New Revision: 314989<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=314989&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=314989&view=rev</a><br>
> Log:<br>
> [clangd] Added async API to run code completion.<br>
><br>
> Summary:<br>
> ClangdServer now provides async code completion API.<br>
> It is still used synchronously by ClangdLSPServer, more work is needed to<br>
> allow processing other requests in parallel while completion (or any other<br>
> request) is running.<br>
><br>
> Reviewers: klimek, bkramer, krasimir<br>
><br>
> Reviewed By: klimek<br>
><br>
> Subscribers: cfe-commits<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D38583" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D38583</a><br>
><br>
> Modified:<br>
> clang-tools-extra/trunk/<wbr>clangd/ClangdLSPServer.cpp<br>
> clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp<br>
> clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h<br>
> clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp<br>
><br>
> Modified: clang-tools-extra/trunk/<wbr>clangd/ClangdLSPServer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-</a><br>
> extra/trunk/clangd/<wbr>ClangdLSPServer.cpp?rev=<wbr>314989&r1=314988&r2=314989&<wbr>view=dif<br>
> f<br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- clang-tools-extra/trunk/<wbr>clangd/ClangdLSPServer.cpp (original)<br>
> +++ clang-tools-extra/trunk/<wbr>clangd/ClangdLSPServer.cpp Thu Oct 5<br>
</div></div>> +++ 10:04:13 2017<br>
<span class="">> @@ -149,6 +149,9 @@ void ClangdLSPServer::onCompletion(<wbr>TextD<br>
> .codeComplete(Params.<wbr>textDocument.uri.file,<br>
> Position{Params.position.line,<br>
> Params.position.character})<br>
> + .get() // FIXME(ibiryukov): This could be made async if we<br>
> + // had an API that would allow to attach callbacks<br>
> to<br>
> + // futures returned by ClangdServer.<br>
> .Value;<br>
><br>
> std::string Completions;<br>
><br>
> Modified: clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-</a><br>
> extra/trunk/clangd/<wbr>ClangdServer.cpp?rev=314989&<wbr>r1=314988&r2=314989&view=diff<br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp (original)<br>
> +++ clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp Thu Oct 5 10:04:13<br>
</span>> +++ 2017<br>
<div><div class="h5">> @@ -194,18 +194,19 @@ std::future<void> ClangdServer::forceRep<br>
> std::move(TaggedFS)); }<br>
><br>
> -Tagged<std::vector<<wbr>CompletionItem>><br>
> +std::future<Tagged<std::<wbr>vector<CompletionItem>>><br>
> ClangdServer::codeComplete(<wbr>PathRef File, Position Pos,<br>
> llvm::Optional<StringRef> OverridenContents,<br>
> IntrusiveRefCntPtr<vfs::<wbr>FileSystem> *UsedFS) {<br>
> - std::string DraftStorage;<br>
> - if (!OverridenContents) {<br>
> + std::string Contents;<br>
> + if (OverridenContents) {<br>
> + Contents = *OverridenContents;<br>
> + } else {<br>
> auto FileContents = DraftMgr.getDraft(File);<br>
> assert(FileContents.Draft &&<br>
> "codeComplete is called for non-added document");<br>
><br>
> - DraftStorage = std::move(*FileContents.Draft)<wbr>;<br>
> - OverridenContents = DraftStorage;<br>
> + Contents = std::move(*FileContents.Draft)<wbr>;<br>
> }<br>
><br>
> auto TaggedFS = FSProvider.<wbr>getTaggedFileSystem(File);<br>
> @@ -215,12 +216,36 @@ ClangdServer::codeComplete(<wbr>PathRef File,<br>
> std::shared_ptr<CppFile> Resources = Units.getFile(File);<br>
> assert(Resources && "Calling completion on non-added file");<br>
><br>
> - auto Preamble = Resources-><wbr>getPossiblyStalePreamble();<br>
> - std::vector<CompletionItem> Result = clangd::codeComplete(<br>
> - File, Resources->getCompileCommand()<wbr>,<br>
> - Preamble ? &Preamble->Preamble : nullptr, *OverridenContents, Pos,<br>
> - TaggedFS.Value, PCHs, SnippetCompletions, Logger);<br>
> - return make_tagged(std::move(Result), TaggedFS.Tag);<br>
> + using PackagedTask =<br>
> + std::packaged_task<Tagged<std:<wbr>:vector<CompletionItem>>()>;<br>
> +<br>
> + // Remember the current Preamble and use it when async task starts<br>
> executing.<br>
> + // At the point when async task starts executing, we may have a<br>
</div></div>> + different // Preamble in Resources. However, we assume the Preamble<br>
> + that we obtain here // is reusable in completion more often.<br>
<span class="">> + std::shared_ptr<const PreambleData> Preamble =<br>
> + Resources-><wbr>getPossiblyStalePreamble();<br>
> + // A task that will be run asynchronously.<br>
> + PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble<br>
> variable.<br>
> + if (!Preamble) {<br>
> + // Maybe we built some preamble before processing this request.<br>
> + Preamble = Resources-><wbr>getPossiblyStalePreamble();<br>
> + }<br>
> + // FIXME(ibiryukov): even if Preamble is non-null, we may want to check<br>
> + // both the old and the new version in case only one of them matches.<br>
> +<br>
> + std::vector<CompletionItem> Result = clangd::codeComplete(<br>
> + File, Resources->getCompileCommand()<wbr>,<br>
> + Preamble ? &Preamble->Preamble : nullptr, Contents, Pos,<br>
> TaggedFS.Value,<br>
> + PCHs, SnippetCompletions, Logger);<br>
> + return make_tagged(std::move(Result), std::move(TaggedFS.Tag));<br>
> + });<br>
> +<br>
> + auto Future = Task.get_future();<br>
> + // FIXME(ibiryukov): to reduce overhead for wrapping the same<br>
</span>> + callable // multiple times, ClangdScheduler should return future<> itself.<br>
<span class="">> + WorkScheduler.addToFront([](<wbr>PackagedTask Task) { Task(); },<br>
</span>> + std::move(Task)); return Future;<br>
<span class="">> }<br>
><br>
> std::vector<tooling::<wbr>Replacement> ClangdServer::formatRange(<wbr>PathRef File,<br>
><br>
> Modified: clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-</a><br>
> extra/trunk/clangd/<wbr>ClangdServer.h?rev=314989&r1=<wbr>314988&r2=314989&view=diff<br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h (original)<br>
> +++ clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h Thu Oct 5 10:04:13<br>
</span>> +++ 2017<br>
<span class="">> @@ -231,19 +231,25 @@ public:<br>
> /// and AST and rebuild them from scratch.<br>
> std::future<void> forceReparse(PathRef File);<br>
><br>
> - /// Run code completion for \p File at \p Pos. If \p OverridenContents is<br>
> not<br>
> - /// None, they will used only for code completion, i.e. no diagnostics<br>
> update<br>
> - /// will be scheduled and a draft for \p File will not be updated.<br>
> - /// If \p OverridenContents is None, contents of the current draft for \p<br>
> File<br>
> - /// will be used.<br>
> - /// If \p UsedFS is non-null, it will be overwritten by vfs::FileSystem<br>
> used<br>
> - /// for completion.<br>
> - /// This method should only be called for currently tracked<br>
> - /// files.<br>
> - Tagged<std::vector<<wbr>CompletionItem>><br>
> + /// Run code completion for \p File at \p Pos.<br>
> + ///<br>
> + /// Request is processed asynchronously. You can use the returned<br>
</span>> + future to /// wait for the results of the async request.<br>
<span class="">> + ///<br>
> + /// If \p OverridenContents is not None, they will used only for code<br>
> + /// completion, i.e. no diagnostics update will be scheduled and a<br>
</span>> + draft for /// \p File will not be updated. If \p OverridenContents is<br>
> + None, contents of /// the current draft for \p File will be used. If<br>
> + \p UsedFS is non-null, it /// will be overwritten by vfs::FileSystem used<br>
<span class="">> for completion.<br>
> + ///<br>
> + /// This method should only be called for currently tracked files.<br>
</span>> + However, it /// is safe to call removeDocument for \p File after this<br>
> + method returns, even /// while returned future is not yet ready.<br>
<span class="">> + std::future<Tagged<std::<wbr>vector<CompletionItem>>><br>
> codeComplete(PathRef File, Position Pos,<br>
> llvm::Optional<StringRef> OverridenContents = llvm::None,<br>
> IntrusiveRefCntPtr<vfs::<wbr>FileSystem> *UsedFS = nullptr);<br>
> +<br>
> /// Get definition of symbol at a specified \p Line and \p Column in \p<br>
> File.<br>
> Tagged<std::vector<Location>> findDefinitions(PathRef File, Position Pos);<br>
><br>
><br>
> Modified: clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-</a><br>
> extra/trunk/unittests/clangd/<wbr>ClangdTests.cpp?rev=314989&r1=<wbr>314988&r2=314989&vi<br>
> ew=diff<br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp (original)<br>
> +++ clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp Thu Oct 5<br>
</span>> +++ 10:04:13 2017<br>
<span class="">> @@ -473,13 +473,13 @@ TEST_F(ClangdVFSTest, CheckVersions) {<br>
> // thread.<br>
> FS.Tag = "123";<br>
> Server.addDocument(FooCpp, SourceContents);<br>
> - EXPECT_EQ(Server.codeComplete(<wbr>FooCpp, Position{0, 0}).Tag, FS.Tag);<br>
> + EXPECT_EQ(Server.codeComplete(<wbr>FooCpp, Position{0, 0}).get().Tag,<br>
</span>> + FS.Tag);<br>
<span class="">> EXPECT_EQ(DiagConsumer.<wbr>lastVFSTag(), FS.Tag);<br>
><br>
> FS.Tag = "321";<br>
> Server.addDocument(FooCpp, SourceContents);<br>
> EXPECT_EQ(DiagConsumer.<wbr>lastVFSTag(), FS.Tag);<br>
> - EXPECT_EQ(Server.codeComplete(<wbr>FooCpp, Position{0, 0}).Tag, FS.Tag);<br>
> + EXPECT_EQ(Server.codeComplete(<wbr>FooCpp, Position{0, 0}).get().Tag,<br>
</span>> + FS.Tag);<br>
<div class="HOEnZb"><div class="h5">> }<br>
><br>
> // Only enable this test on Unix<br>
> @@ -631,7 +631,7 @@ int b = ;<br>
><br>
> {<br>
> auto CodeCompletionResults1 =<br>
> - Server.codeComplete(FooCpp, CompletePos, None).Value;<br>
> + Server.codeComplete(FooCpp, CompletePos, None).get().Value;<br>
> EXPECT_TRUE(ContainsItem(<wbr>CodeCompletionResults1, "aba"));<br>
> EXPECT_FALSE(ContainsItem(<wbr>CodeCompletionResults1, "cbc"));<br>
> }<br>
> @@ -641,6 +641,7 @@ int b = ;<br>
> Server<br>
> .codeComplete(FooCpp, CompletePos,<br>
> StringRef(<wbr>OverridenSourceContents))<br>
> + .get()<br>
> .Value;<br>
> EXPECT_TRUE(ContainsItem(<wbr>CodeCompletionResultsOverriden<wbr>, "cbc"));<br>
> EXPECT_FALSE(ContainsItem(<wbr>CodeCompletionResultsOverriden<wbr>, "aba"));<br>
> @@ -648,7 +649,7 @@ int b = ;<br>
><br>
> {<br>
> auto CodeCompletionResults2 =<br>
> - Server.codeComplete(FooCpp, CompletePos, None).Value;<br>
> + Server.codeComplete(FooCpp, CompletePos, None).get().Value;<br>
> EXPECT_TRUE(ContainsItem(<wbr>CodeCompletionResults2, "aba"));<br>
> EXPECT_FALSE(ContainsItem(<wbr>CodeCompletionResults2, "cbc"));<br>
> }<br>
> @@ -840,7 +841,13 @@ int d;<br>
> AddDocument(FileIndex);<br>
><br>
> Position Pos{LineDist(RandGen), ColumnDist(RandGen)};<br>
> - Server.codeComplete(FilePaths[<wbr>FileIndex], Pos);<br>
> + // FIXME(ibiryukov): Also test async completion requests.<br>
> + // Simply putting CodeCompletion into async requests now would make<br>
> + // tests slow, since there's no way to cancel previous completion<br>
> + // requests as opposed to AddDocument/RemoveDocument, which are<br>
> implicitly<br>
> + // cancelled by any subsequent AddDocument/RemoveDocument request to<br>
> the<br>
> + // same file.<br>
> + Server.codeComplete(FilePaths[<wbr>FileIndex], Pos).wait();<br>
> };<br>
><br>
> auto FindDefinitionsRequest = [&]() {<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</div>