[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

Build now is OK. Thanks @ilya-biryukov


Repository:
  rL LLVM

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via cfe-commits]

Sorry about that, fixed in r325254.
I keep hitting this MSVC compilation error all the time.


On Thu, Feb 15, 2018 at 4:26 PM Carlos Alberto Enciso via Phabricator <
revi...@reviews.llvm.org> wrote:

> CarlosAlbertoEnciso added a comment.
>
> Hi,
>
> It seems that your submission broke the Windows/Linux builds for 'clangd'.
>
> The generated error message is:
>
>   C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\VC\include\future(263): error C2512:
> 'llvm::Expected>>':
> no appropriate default constructor available
> [C:\j\w\opensource\opensource_to_sce_merge\build\tools\clang\tools\extra\unittests\clangd\ClangdTests.vcxproj]
>
>   C:\xxx\llvm\tools\clang\tools\extra\clangd\ClangdServer.h(231): note:
> see declaration of
> 'llvm::Expected>>'
>   C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\VC\include\future(255): note: while compiling class template member
> function
> 'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)'
>   C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\VC\include\future(1491): note: see reference to function template
> instantiation
> 'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)'
> being compiled
>   C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\VC\include\future(989): note: see reference to class template
> instantiation 'std::_Associated_state<_Ty>' being compiled
>   C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\VC\include\future(987): note: while compiling class template member
> function 'bool std::_State_manager<_Ty>::valid(void) throw() const'
>
> Thanks
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D43227
>
>
>
>

-- 
Regards,
Ilya Biryukov
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

Hi,

It seems that your submission broke the Windows/Linux builds for 'clangd'.

The generated error message is:

  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(263): 
error C2512: 
'llvm::Expected>>':
 no appropriate default constructor available 
[C:\j\w\opensource\opensource_to_sce_merge\build\tools\clang\tools\extra\unittests\clangd\ClangdTests.vcxproj]
  
  C:\xxx\llvm\tools\clang\tools\extra\clangd\ClangdServer.h(231): note: see 
declaration of 
'llvm::Expected>>'
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(255): 
note: while compiling class template member function 
'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)'
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(1491): 
note: see reference to function template instantiation 
'std::_Associated_state<_Ty>::_Associated_state(std::_Deleter_base<_Ty> *)' 
being compiled
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(989): 
note: see reference to class template instantiation 
'std::_Associated_state<_Ty>' being compiled
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\future(987): 
note: while compiling class template member function 'bool 
std::_State_manager<_Ty>::valid(void) throw() const'

Thanks


Repository:
  rL LLVM

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325233: [clangd] Make functions of ClangdServer 
callback-based (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43227

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
  clang-tools-extra/trunk/unittests/clangd/SyncAPI.h
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -96,7 +96,7 @@
 }
 
 std::string dumpASTWithoutMemoryLocs(ClangdServer , PathRef File) {
-  auto DumpWithMemLocs = Server.dumpAST(File);
+  auto DumpWithMemLocs = runDumpAST(Server, File);
   return replacePtrsInDump(DumpWithMemLocs);
 }
 
@@ -432,17 +432,17 @@
   // Clang can't parse command args in that case, but we shouldn't crash.
   Server.addDocument(FooCpp, "int main() {}");
 
-  EXPECT_EQ(Server.dumpAST(FooCpp), "");
-  EXPECT_ERROR(Server.findDefinitions(FooCpp, Position()));
-  EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position()));
-  EXPECT_ERROR(Server.rename(FooCpp, Position(), "new_name"));
+  EXPECT_EQ(runDumpAST(Server, FooCpp), "");
+  EXPECT_ERROR(runFindDefinitions(Server, FooCpp, Position()));
+  EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position()));
+  EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name"));
   // FIXME: codeComplete and signatureHelp should also return errors when they
   // can't parse the file.
   EXPECT_THAT(
   runCodeComplete(Server, FooCpp, Position(), clangd::CodeCompleteOptions())
   .Value.items,
   IsEmpty());
-  auto SigHelp = Server.signatureHelp(FooCpp, Position());
+  auto SigHelp = runSignatureHelp(Server, FooCpp, Position());
   ASSERT_TRUE(bool(SigHelp)) << "signatureHelp returned an error";
   EXPECT_THAT(SigHelp->Value.signatures, IsEmpty());
 }
@@ -646,7 +646,7 @@
   Pos.line = LineDist(RandGen);
   Pos.character = ColumnDist(RandGen);
 
-  ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+  ASSERT_TRUE(!!runFindDefinitions(Server, FilePaths[FileIndex], Pos));
 };
 
 std::vector> AsyncRequests = {
@@ -768,8 +768,8 @@
   public:
 std::atomic Count = {0};
 
- NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse)
- : StartSecondReparse(std::move(StartSecondReparse)) {}
+NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse)
+: StartSecondReparse(std::move(StartSecondReparse)) {}
 
 void onDiagnosticsReady(PathRef,
 Tagged) override {
Index: clang-tools-extra/trunk/unittests/clangd/SyncAPI.h
===
--- clang-tools-extra/trunk/unittests/clangd/SyncAPI.h
+++ clang-tools-extra/trunk/unittests/clangd/SyncAPI.h
@@ -22,6 +22,22 @@
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName);
+
+std::string runDumpAST(ClangdServer , PathRef File);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -10,6 +10,7 @@
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -249,7 +250,8 @@
   FS.Files[FooCpp] = "";
 
   Server.addDocument(FooCpp, SourceAnnotations.code());
-  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
 
   EXPECT_THAT(Locations->Value,
Index: 

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 134406.
ilya-biryukov added a comment.

- Rebase onto head


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -10,6 +10,7 @@
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -249,7 +250,8 @@
   FS.Files[FooCpp] = "";
 
   Server.addDocument(FooCpp, SourceAnnotations.code());
-  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
 
   EXPECT_THAT(Locations->Value,
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,6 +22,22 @@
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName);
+
+std::string runDumpAST(ClangdServer , PathRef File);
+
 } // namespace clangd
 } // namespace clang
 
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -17,7 +17,9 @@
 ///T Result;
 ///someAsyncFunc(Param1, Param2, /*Callback=*/capture(Result));
 template  struct CaptureProxy {
-  CaptureProxy(T ) : Target() {}
+  CaptureProxy(llvm::Optional ) : Target() {
+assert(!Target.hasValue());
+  }
 
   CaptureProxy(const CaptureProxy &) = delete;
   CaptureProxy =(const CaptureProxy &) = delete;
@@ -39,27 +41,63 @@
 if (!Target)
   return;
 assert(Future.valid() && "conversion to callback was not called");
-*Target = Future.get();
+assert(!Target->hasValue());
+Target->emplace(Future.get());
   }
 
 private:
-  T *Target;
+  llvm::Optional *Target;
   std::promise Promise;
   std::future Future;
 };
 
-template  CaptureProxy capture(T ) {
+template  CaptureProxy capture(llvm::Optional ) {
   return CaptureProxy(Target);
 }
 } // namespace
 
 Tagged
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents) {
-  Tagged Result;
+  llvm::Optional Result;
   Server.codeComplete(File, Pos, Opts, capture(Result), OverridenContents);
-  return Result;
+  return std::move(*Result);
+}
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents) {
+  llvm::Optional> Result;
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
+  return std::move(*Result);
+}
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos) {
+  llvm::Optional>> Result;
+  Server.findDefinitions(File, Pos, capture(Result));
+  return std::move(*Result);
+}
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos) {
+  llvm::Optional>> Result;
+  Server.findDocumentHighlights(File, Pos, capture(Result));
+  return std::move(*Result);
+}
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, capture(Result));
+  return std::move(*Result);
+}
+
+std::string runDumpAST(ClangdServer , PathRef File) {
+  llvm::Optional Result;
+  Server.dumpAST(File, capture(Result));
+  return std::move(*Result);
 }
 
 } // namespace clangd
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -626,7 +626,7 @@
   Annotations Test(Text);
   

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.

lg




Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

ilya-biryukov wrote:
> sammccall wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > nit: I'd probably use a different name than `Callback` for this 
> > > > > parameter for clarity.
> > > > I actually think we should keep it this way. It's a bit tricky to 
> > > > grasp, but it has two important advantages:
> > > >   - Makes referencing `Callback` from outer scope impossible
> > > >   - saves us from coming up with names for that superficial variable, 
> > > > i.e. should it be called `InnerCallback`, `Callback2`, `C` or something 
> > > > else?
> > > > 
> > > > Is it that too confusing or do you feel we can keep it?
> > > > Makes referencing Callback from outer scope impossible. 
> > > For lambdas, I think we should rely on captures for this instead of the 
> > > parameter names.
> > > >saves us from coming up with names for that superficial variable, i.e. 
> > > >should it be called InnerCallback, Callback2, C or something else?
> > > I thought this is a common practice with llvm coding style :P I would 
> > > vote for `C` :) 
> > > 
> > > > Is it that too confusing or do you feel we can keep it?
> > > It is a bit confusing when I first looked at it, but nothing is *too* 
> > > confusing as long as the code is correct ;) I just think it would make 
> > > the code easier to read.
> > I agree with you both :-)
> > Conceptually, this *is* a capture - BindWithForward simulates move-capture 
> > that C++11 doesn't have native syntax for. So the same name seems 
> > appropriate to me - you want shadowing  for the same reasons you want 
> > captures to shadow.
> I'll leave as is in this review, we have other callsites, doing the same 
> thing, that we don't touch here. If we decide to change it, we should change 
> all of them in one go.
> I'm not opposed to the change, but like the current style a bit more.
Feel free to land without changing this; this is a nit anyway ;)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 134169.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.

- Use llvm::Optional<> in capture() helper to avoid confusion with 
llvm::Expected.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -10,6 +10,7 @@
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -249,7 +250,8 @@
   FS.Files[FooCpp] = "";
 
   Server.addDocument(FooCpp, SourceAnnotations.code());
-  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
 
   EXPECT_THAT(Locations->Value,
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,6 +22,22 @@
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName);
+
+std::string runDumpAST(ClangdServer , PathRef File);
+
 } // namespace clangd
 } // namespace clang
 
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -17,7 +17,9 @@
 ///T Result;
 ///someAsyncFunc(Param1, Param2, /*Callback=*/capture(Result));
 template  struct CaptureProxy {
-  CaptureProxy(T ) : Target() {}
+  CaptureProxy(llvm::Optional ) : Target() {
+assert(!Target.hasValue());
+  }
 
   CaptureProxy(const CaptureProxy &) = delete;
   CaptureProxy =(const CaptureProxy &) = delete;
@@ -39,27 +41,63 @@
 if (!Target)
   return;
 assert(Future.valid() && "conversion to callback was not called");
-*Target = Future.get();
+assert(!Target->hasValue());
+Target->emplace(Future.get());
   }
 
 private:
-  T *Target;
+  llvm::Optional *Target;
   std::promise Promise;
   std::future Future;
 };
 
-template  CaptureProxy capture(T ) {
+template  CaptureProxy capture(llvm::Optional ) {
   return CaptureProxy(Target);
 }
 } // namespace
 
 Tagged
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents) {
-  Tagged Result;
+  llvm::Optional Result;
   Server.codeComplete(File, Pos, Opts, capture(Result), OverridenContents);
-  return Result;
+  return std::move(*Result);
+}
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents) {
+  llvm::Optional> Result;
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
+  return std::move(*Result);
+}
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos) {
+  llvm::Optional>> Result;
+  Server.findDefinitions(File, Pos, capture(Result));
+  return std::move(*Result);
+}
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos) {
+  llvm::Optional>> Result;
+  Server.findDocumentHighlights(File, Pos, capture(Result));
+  return std::move(*Result);
+}
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, capture(Result));
+  return std::move(*Result);
+}
+
+std::string runDumpAST(ClangdServer , PathRef File) {
+  llvm::Optional Result;
+  Server.dumpAST(File, capture(Result));
+  return std::move(*Result);
 }
 
 } // namespace clangd
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- 

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D43227#1006584, @sammccall wrote:

> I wonder whether we want to introduce `using Callback = 
> UniqueFunction` for readability at some point...


I agree that's a good idea, will come up with a CL doing that.




Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

sammccall wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > nit: I'd probably use a different name than `Callback` for this 
> > > > parameter for clarity.
> > > I actually think we should keep it this way. It's a bit tricky to grasp, 
> > > but it has two important advantages:
> > >   - Makes referencing `Callback` from outer scope impossible
> > >   - saves us from coming up with names for that superficial variable, 
> > > i.e. should it be called `InnerCallback`, `Callback2`, `C` or something 
> > > else?
> > > 
> > > Is it that too confusing or do you feel we can keep it?
> > > Makes referencing Callback from outer scope impossible. 
> > For lambdas, I think we should rely on captures for this instead of the 
> > parameter names.
> > >saves us from coming up with names for that superficial variable, i.e. 
> > >should it be called InnerCallback, Callback2, C or something else?
> > I thought this is a common practice with llvm coding style :P I would vote 
> > for `C` :) 
> > 
> > > Is it that too confusing or do you feel we can keep it?
> > It is a bit confusing when I first looked at it, but nothing is *too* 
> > confusing as long as the code is correct ;) I just think it would make the 
> > code easier to read.
> I agree with you both :-)
> Conceptually, this *is* a capture - BindWithForward simulates move-capture 
> that C++11 doesn't have native syntax for. So the same name seems appropriate 
> to me - you want shadowing  for the same reasons you want captures to shadow.
I'll leave as is in this review, we have other callsites, doing the same thing, 
that we don't touch here. If we decide to change it, we should change all of 
them in one go.
I'm not opposed to the change, but like the current style a bit more.



Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);

sammccall wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > I'd expect this to be checked by callers. Would `return 
> > > > std::move(Result);` work?
> > > The reason why we need it is because `capture(Result)` writes return 
> > > value of the callback to the `Result` variable. But we have to first 
> > > check the default-constructed value that was there **before** calling 
> > > `Server.signatureHelp`
> > I think we should probably fix the behavior of `capture` since this changes 
> > the behavior of `llvm::Expected` in user code - the check is there to make 
> > sure users always check the status. But here we always do this for users.
> +1
> 
> I suggested `capture(T&)` was the right API because I thought it'd be used 
> directly by test code (but we wrap it here) and because I thought T would be 
> default-constructible without problems (but it's not).
> 
> I'd suggest changing to `capture(Optional&)` instead, so the caller can 
> just create an empty optional. That makes the callsites here slightly less 
> obscure.
The users still have to check the returned value. We only check the 
**default-constructed** value. After `capture()` runs `Result` is reassigned 
and now has to be checked again (and this should be done by the users).
Replaced with `capture(Optional&)` to make it absolutely clear we're doing 
the right thing there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, suggest a tweak to capture() though.

I wonder whether we want to introduce `using Callback = 
UniqueFunction` for readability at some point...




Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > nit: I'd probably use a different name than `Callback` for this parameter 
> > > for clarity.
> > I actually think we should keep it this way. It's a bit tricky to grasp, 
> > but it has two important advantages:
> >   - Makes referencing `Callback` from outer scope impossible
> >   - saves us from coming up with names for that superficial variable, i.e. 
> > should it be called `InnerCallback`, `Callback2`, `C` or something else?
> > 
> > Is it that too confusing or do you feel we can keep it?
> > Makes referencing Callback from outer scope impossible. 
> For lambdas, I think we should rely on captures for this instead of the 
> parameter names.
> >saves us from coming up with names for that superficial variable, i.e. 
> >should it be called InnerCallback, Callback2, C or something else?
> I thought this is a common practice with llvm coding style :P I would vote 
> for `C` :) 
> 
> > Is it that too confusing or do you feel we can keep it?
> It is a bit confusing when I first looked at it, but nothing is *too* 
> confusing as long as the code is correct ;) I just think it would make the 
> code easier to read.
I agree with you both :-)
Conceptually, this *is* a capture - BindWithForward simulates move-capture that 
C++11 doesn't have native syntax for. So the same name seems appropriate to me 
- you want shadowing  for the same reasons you want captures to shadow.



Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > I'd expect this to be checked by callers. Would `return 
> > > std::move(Result);` work?
> > The reason why we need it is because `capture(Result)` writes return value 
> > of the callback to the `Result` variable. But we have to first check the 
> > default-constructed value that was there **before** calling 
> > `Server.signatureHelp`
> I think we should probably fix the behavior of `capture` since this changes 
> the behavior of `llvm::Expected` in user code - the check is there to make 
> sure users always check the status. But here we always do this for users.
+1

I suggested `capture(T&)` was the right API because I thought it'd be used 
directly by test code (but we wrap it here) and because I thought T would be 
default-constructible without problems (but it's not).

I'd suggest changing to `capture(Optional&)` instead, so the caller can just 
create an empty optional. That makes the callsites here slightly less obscure.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:194
+if (!Replacements)
+  return replyError(ErrorCode::InternalError,
+llvm::toString(Replacements.takeError()));

ilya-biryukov wrote:
> ioeric wrote:
> > nit: since we are not spelling out the return type here, it might be 
> > clearer if we do:
> > ```
> > replyError(...);
> > return;
> > ```
> > 
> > `return replyError(...)` makes me wonder what the return type is.
> This trick was used in the code before (there are usages below), so I figured 
> it's ok to do this.
> If it's confusing I'll change it everywhere.
I guess it's not confusing after figuring out what `replyError` returns. There 
is obviously tradeoff between LOC and readability. Either is not too bad. So up 
to you :)



Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

ilya-biryukov wrote:
> ioeric wrote:
> > nit: I'd probably use a different name than `Callback` for this parameter 
> > for clarity.
> I actually think we should keep it this way. It's a bit tricky to grasp, but 
> it has two important advantages:
>   - Makes referencing `Callback` from outer scope impossible
>   - saves us from coming up with names for that superficial variable, i.e. 
> should it be called `InnerCallback`, `Callback2`, `C` or something else?
> 
> Is it that too confusing or do you feel we can keep it?
> Makes referencing Callback from outer scope impossible. 
For lambdas, I think we should rely on captures for this instead of the 
parameter names.
>saves us from coming up with names for that superficial variable, i.e. should 
>it be called InnerCallback, Callback2, C or something else?
I thought this is a common practice with llvm coding style :P I would vote for 
`C` :) 

> Is it that too confusing or do you feel we can keep it?
It is a bit confusing when I first looked at it, but nothing is *too* confusing 
as long as the code is correct ;) I just think it would make the code easier to 
read.



Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);

ilya-biryukov wrote:
> ioeric wrote:
> > I'd expect this to be checked by callers. Would `return std::move(Result);` 
> > work?
> The reason why we need it is because `capture(Result)` writes return value of 
> the callback to the `Result` variable. But we have to first check the 
> default-constructed value that was there **before** calling 
> `Server.signatureHelp`
I think we should probably fix the behavior of `capture` since this changes the 
behavior of `llvm::Expected` in user code - the check is there to make sure 
users always check the status. But here we always do this for users.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 134080.
ilya-biryukov added a comment.

- Capture only needed vars in lambda, not everything.
- Rebase onto head.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -10,6 +10,7 @@
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -249,7 +250,8 @@
   FS.Files[FooCpp] = "";
 
   Server.addDocument(FooCpp, SourceAnnotations.code());
-  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
 
   EXPECT_THAT(Locations->Value,
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,6 +22,22 @@
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName);
+
+std::string runDumpAST(ClangdServer , PathRef File);
+
 } // namespace clangd
 } // namespace clang
 
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -62,5 +62,47 @@
   return Result;
 }
 
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents) {
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
+  return Result;
+}
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos) {
+  llvm::Expected> Result =
+  Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.findDefinitions(File, Pos, capture(Result));
+  return Result;
+}
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos) {
+  llvm::Expected> Result =
+  Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.findDocumentHighlights(File, Pos, capture(Result));
+  return Result;
+}
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName) {
+  llvm::Expected Result =
+  std::vector();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.rename(File, Pos, NewName, capture(Result));
+  return Result;
+}
+
+std::string runDumpAST(ClangdServer , PathRef File) {
+  std::string Result;
+  Server.dumpAST(File, capture(Result));
+  return Result;
+}
+
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -626,7 +626,7 @@
   Annotations Test(Text);
   Server.addDocument(File, Test.code());
   EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
-  auto R = Server.signatureHelp(File, Test.point());
+  auto R = runSignatureHelp(Server, File, Test.point());
   assert(R);
   return R.get().Value;
 }
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -112,7 +112,7 @@
 }
 
 std::string dumpASTWithoutMemoryLocs(ClangdServer , PathRef File) {
-  auto DumpWithMemLocs = Server.dumpAST(File);
+  auto DumpWithMemLocs = runDumpAST(Server, File);
   return replacePtrsInDump(DumpWithMemLocs);
 }
 
@@ -450,17 +450,17 @@
   // Clang can't parse command args in that case, but we shouldn't crash.
   Server.addDocument(FooCpp, "int main() {}");
 
-  EXPECT_EQ(Server.dumpAST(FooCpp), "");
-  

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:194
+if (!Replacements)
+  return replyError(ErrorCode::InternalError,
+llvm::toString(Replacements.takeError()));

ioeric wrote:
> nit: since we are not spelling out the return type here, it might be clearer 
> if we do:
> ```
> replyError(...);
> return;
> ```
> 
> `return replyError(...)` makes me wonder what the return type is.
This trick was used in the code before (there are usages below), so I figured 
it's ok to do this.
If it's confusing I'll change it everywhere.



Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

ioeric wrote:
> nit: I'd probably use a different name than `Callback` for this parameter for 
> clarity.
I actually think we should keep it this way. It's a bit tricky to grasp, but it 
has two important advantages:
  - Makes referencing `Callback` from outer scope impossible
  - saves us from coming up with names for that superficial variable, i.e. 
should it be called `InnerCallback`, `Callback2`, `C` or something else?

Is it that too confusing or do you feel we can keep it?



Comment at: clangd/ClangdServer.cpp:441
 
-  using RetType = llvm::Expected>;
-  auto Action = [=](llvm::Expected InpAST) -> RetType {
+  auto Action = [=](decltype(Callback) Callback,
+llvm::Expected InpAST) {

ioeric wrote:
> Consider spelling out the captured values, just in case new variables which 
> are not expected to be captured are added in the future.
Thanks for spotting this!



Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);

ioeric wrote:
> I'd expect this to be checked by callers. Would `return std::move(Result);` 
> work?
The reason why we need it is because `capture(Result)` writes return value of 
the callback to the `Result` variable. But we have to first check the 
default-constructed value that was there **before** calling 
`Server.signatureHelp`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:194
+if (!Replacements)
+  return replyError(ErrorCode::InternalError,
+llvm::toString(Replacements.takeError()));

nit: since we are not spelling out the return type here, it might be clearer if 
we do:
```
replyError(...);
return;
```

`return replyError(...)` makes me wonder what the return type is.



Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+   llvm::Expected IP) {

nit: I'd probably use a different name than `Callback` for this parameter for 
clarity.



Comment at: clangd/ClangdServer.cpp:441
 
-  using RetType = llvm::Expected>;
-  auto Action = [=](llvm::Expected InpAST) -> RetType {
+  auto Action = [=](decltype(Callback) Callback,
+llvm::Expected InpAST) {

Consider spelling out the captured values, just in case new variables which are 
not expected to be captured are added in the future.



Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);

I'd expect this to be checked by callers. Would `return std::move(Result);` 
work?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, ioeric, hokein.
Herald added subscribers: jkorous-apple, klimek.

As a consequence, all LSP operations are now handled asynchronously,
i.e. they never block the main processing thread. However, if
-run-synchronously flag is specified, clangd still runs everything on
the main thread.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h

Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -22,6 +22,22 @@
 runCodeComplete(ClangdServer , PathRef File, Position Pos,
 clangd::CodeCompleteOptions Opts,
 llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents = llvm::None);
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos);
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName);
+
+std::string runDumpAST(ClangdServer , PathRef File);
+
 } // namespace clangd
 } // namespace clang
 
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -62,5 +62,47 @@
   return Result;
 }
 
+llvm::Expected
+runSignatureHelp(ClangdServer , PathRef File, Position Pos,
+ llvm::Optional OverridenContents) {
+  llvm::Expected Result = Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
+  return Result;
+}
+
+llvm::Expected>
+runFindDefinitions(ClangdServer , PathRef File, Position Pos) {
+  llvm::Expected> Result =
+  Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.findDefinitions(File, Pos, capture(Result));
+  return Result;
+}
+
+llvm::Expected>
+runFindDocumentHighlights(ClangdServer , PathRef File, Position Pos) {
+  llvm::Expected> Result =
+  Tagged();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.findDocumentHighlights(File, Pos, capture(Result));
+  return Result;
+}
+
+llvm::Expected
+runRename(ClangdServer , PathRef File, Position Pos, StringRef NewName) {
+  llvm::Expected Result =
+  std::vector();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.rename(File, Pos, NewName, capture(Result));
+  return Result;
+}
+
+std::string runDumpAST(ClangdServer , PathRef File) {
+  std::string Result;
+  Server.dumpAST(File, capture(Result));
+  return Result;
+}
+
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -626,7 +626,7 @@
   Annotations Test(Text);
   Server.addDocument(File, Test.code());
   EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
-  auto R = Server.signatureHelp(File, Test.point());
+  auto R = runSignatureHelp(Server, File, Test.point());
   assert(R);
   return R.get().Value;
 }
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -112,7 +112,7 @@
 }
 
 std::string dumpASTWithoutMemoryLocs(ClangdServer , PathRef File) {
-  auto DumpWithMemLocs = Server.dumpAST(File);
+  auto DumpWithMemLocs = runDumpAST(Server, File);
   return replacePtrsInDump(DumpWithMemLocs);
 }
 
@@ -450,17 +450,17 @@
   // Clang can't parse command args in that case, but we shouldn't crash.
   Server.addDocument(FooCpp, "int main() {}");
 
-  EXPECT_EQ(Server.dumpAST(FooCpp), "");
-  EXPECT_ERROR(Server.findDefinitions(FooCpp, Position{0, 0}));
-  EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position{0, 0}));
-  EXPECT_ERROR(Server.rename(FooCpp, Position{0, 0}, "new_name"));
+  EXPECT_EQ(runDumpAST(Server, FooCpp), "");
+  EXPECT_ERROR(runFindDefinitions(Server, FooCpp, Position{0, 0}));
+  EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position{0, 0}));
+  EXPECT_ERROR(runRename(Server, FooCpp, Position{0, 0}, "new_name"));
   // FIXME: codeComplete and signatureHelp should also return errors when they
   // can't parse the file.