[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

[Sam McCall via Phabricator via cfe-commits]

sammccall added subscribers: ilya-biryukov, jkorous.
sammccall added a comment.

Couple of thoughts. (Technically I'm out on leave so will let Jan/Ilya
review implementation and happy with whatever you decide)

Enabling

- negotiating LSP extensions is probably better done in the "capabilities"

message exchange than as a command-line flag. Generally, we want this
extension on if the *client* is aware of it. Roughly, the client
capabilities are owned by the client, and the flags are owned by the *user*.

- for simplicity, we could always enable this, unless we really think the

message size is a problem, or are worried about conflicts with future LSP
versions

Naming

- elsewhere in clangd we settled on calling a "Fix" what clang calls a

"FixItHint". The latter is long/awkward/jargon, and often gets shortened to
"FixIt" which isn't obviously a noun. The former mostly has its plain
English meaning. I'd prefer "fix" in the protocol/flags, for the same
reasons.

- obviously feel free to give these any name you prefer in your UI!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50415



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

[PATCH] D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: ioeric, klimek.
Herald added a subscriber: cfe-commits.

Rather than hold all the JSON source in memory, as well as the YAML structures
needed to index into it, we parse eagerly into CompileCommand structures, which
simplifies the implementation a lot.

Possibly at the cost of performance/memory usage, need to measure that.
(I'd expect that eager parsing with string deduplication would be fine at least)


Repository:
  rC Clang

https://reviews.llvm.org/D50439

Files:
  include/clang/Tooling/JSONCompilationDatabase.h
  lib/Tooling/JSONCompilationDatabase.cpp
  test/Index/skip-parsed-bodies/compile_commands.json
  test/Index/skip-parsed-bodies/compile_commands.test
  test/Index/skip-parsed-bodies/lit.local.cfg

Index: test/Index/skip-parsed-bodies/lit.local.cfg
===
--- test/Index/skip-parsed-bodies/lit.local.cfg
+++ test/Index/skip-parsed-bodies/lit.local.cfg
@@ -1 +1 @@
-config.suffixes = ['.json']
+config.suffixes = ['.test']
Index: test/Index/skip-parsed-bodies/compile_commands.test
===
--- test/Index/skip-parsed-bodies/compile_commands.test
+++ test/Index/skip-parsed-bodies/compile_commands.test
@@ -1,22 +1,4 @@
-[
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t1.cpp",
-  "file": "t1.cpp"
-},
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t2.cpp -DBLAH",
-  "file": "t2.cpp"
-},
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t3.cpp -DBLAH",
-  "file": "t2.cpp"
-}
-]
-
-// RUN: c-index-test -index-compile-db %s | FileCheck %s
+// RUN: c-index-test -index-compile-db %S/compile_commands.json | FileCheck %s
 
 // CHECK:  [startedTranslationUnit]
 // CHECK-NEXT: [enteredMainFile]: t1.cpp
@@ -71,3 +53,4 @@
 // CHECK:  [indexDeclaration]: kind: function | name: imp_foo | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
 // CHECK-NOT:  [indexEntityReference]: kind: variable | name: some_val |
 // CHECK-NOT:  [diagnostic]: {{.*}} undeclared identifier
+
Index: test/Index/skip-parsed-bodies/compile_commands.json
===
--- test/Index/skip-parsed-bodies/compile_commands.json
+++ test/Index/skip-parsed-bodies/compile_commands.json
@@ -15,59 +15,3 @@
   "file": "t2.cpp"
 }
 ]
-
-// RUN: c-index-test -index-compile-db %s | FileCheck %s
-
-// CHECK:  [startedTranslationUnit]
-// CHECK-NEXT: [enteredMainFile]: t1.cpp
-// CHECK:  [indexDeclaration]: kind: c++-instance-method | name: method_decl | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:9:27
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 1 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: namespace | name: NS |
-// CHECK-NEXT: [indexEntityReference]: kind: c++-class | name: C |
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:15:5
-// CHECK-NEXT: [indexDeclaration]: kind: function | name: foo1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:19:5
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val1'
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val2'
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val3'
-
-// CHECK-NEXT: [startedTranslationUnit]
-// CHECK-NEXT: [enteredMainFile]: t2.cpp
-// CHECK:  [indexDeclaration]: kind: c++-instance-method | name: method_decl | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isContainer: skipped
-// CHECK-NEXT: [indexEntityReference]: kind: namespace | name: NS |
-// CHECK-NEXT: [indexEntityReference]: kind: c++-class | name: C |
-// CHECK-NEXT: [indexDeclaration]: kind: function | name: foo1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
-// CHECK-NEXT: [ppIncludedFile]: ./pragma_once.h
-// CHECK-NEXT: [indexDeclaration]: kind: function | n

[PATCH] D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 159690.
sammccall added a comment.

(just updating description)


Repository:
  rC Clang

https://reviews.llvm.org/D50439

Files:
  include/clang/Tooling/JSONCompilationDatabase.h
  lib/Tooling/JSONCompilationDatabase.cpp
  test/Index/skip-parsed-bodies/compile_commands.json
  test/Index/skip-parsed-bodies/compile_commands.test
  test/Index/skip-parsed-bodies/lit.local.cfg

Index: test/Index/skip-parsed-bodies/lit.local.cfg
===
--- test/Index/skip-parsed-bodies/lit.local.cfg
+++ test/Index/skip-parsed-bodies/lit.local.cfg
@@ -1 +1 @@
-config.suffixes = ['.json']
+config.suffixes = ['.test']
Index: test/Index/skip-parsed-bodies/compile_commands.test
===
--- test/Index/skip-parsed-bodies/compile_commands.test
+++ test/Index/skip-parsed-bodies/compile_commands.test
@@ -1,22 +1,4 @@
-[
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t1.cpp",
-  "file": "t1.cpp"
-},
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t2.cpp -DBLAH",
-  "file": "t2.cpp"
-},
-{
-  "directory": ".",
-  "command": "/usr/bin/clang++ -fsyntax-only -fno-ms-compatibility -fno-delayed-template-parsing t3.cpp -DBLAH",
-  "file": "t2.cpp"
-}
-]
-
-// RUN: c-index-test -index-compile-db %s | FileCheck %s
+// RUN: c-index-test -index-compile-db %S/compile_commands.json | FileCheck %s
 
 // CHECK:  [startedTranslationUnit]
 // CHECK-NEXT: [enteredMainFile]: t1.cpp
@@ -71,3 +53,4 @@
 // CHECK:  [indexDeclaration]: kind: function | name: imp_foo | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
 // CHECK-NOT:  [indexEntityReference]: kind: variable | name: some_val |
 // CHECK-NOT:  [diagnostic]: {{.*}} undeclared identifier
+
Index: test/Index/skip-parsed-bodies/compile_commands.json
===
--- test/Index/skip-parsed-bodies/compile_commands.json
+++ test/Index/skip-parsed-bodies/compile_commands.json
@@ -15,59 +15,3 @@
   "file": "t2.cpp"
 }
 ]
-
-// RUN: c-index-test -index-compile-db %s | FileCheck %s
-
-// CHECK:  [startedTranslationUnit]
-// CHECK-NEXT: [enteredMainFile]: t1.cpp
-// CHECK:  [indexDeclaration]: kind: c++-instance-method | name: method_decl | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:9:27
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 1 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: namespace | name: NS |
-// CHECK-NEXT: [indexEntityReference]: kind: c++-class | name: C |
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:15:5
-// CHECK-NEXT: [indexDeclaration]: kind: function | name: foo1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:19:5
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val1'
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val2'
-// CHECK-NEXT: [diagnostic]: {{.*}} undeclared identifier 'undef_val3'
-
-// CHECK-NEXT: [startedTranslationUnit]
-// CHECK-NEXT: [enteredMainFile]: t2.cpp
-// CHECK:  [indexDeclaration]: kind: c++-instance-method | name: method_decl | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isRedecl: 0 | isDef: 0 | isContainer: 0
-// CHECK-NEXT: [indexDeclaration]: kind: c++-instance-method | name: method_def2 | {{.*}} | isContainer: skipped
-// CHECK-NEXT: [indexEntityReference]: kind: namespace | name: NS |
-// CHECK-NEXT: [indexEntityReference]: kind: c++-class | name: C |
-// CHECK-NEXT: [indexDeclaration]: kind: function | name: foo1 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: skipped
-// CHECK-NEXT: [ppIncludedFile]: ./pragma_once.h
-// CHECK-NEXT: [indexDeclaration]: kind: function | name: foo2 | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {{.*}} | loc: ./t.h:25:5
-// CHECK:  [indexDeclaration]: kind: c++-instance-method | name: tsmeth | {{.*}} | isRedecl: 0 | isDef: 1 | isContainer: 1
-// CHECK-NEXT: [indexEntityReference]: kind: variable | name: some_val | {

[PATCH] D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser.

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall added a comment.

Nevermind, this is like 5x slower to load the chromium CDB. probably because of 
all the little non-arena allocations.
I'll profile and fix it when I get bored again.


Repository:
  rC Clang

https://reviews.llvm.org/D50439



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

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:105
+size_t MemIndex::estimateMemoryUsage() {
+  size_t Bytes = Index.size() * sizeof(std::pair);
+  return Bytes / (1000 * 1000);

access to Index needs to be guarded by mutex



Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:105
+size_t MemIndex::estimateMemoryUsage() {
+  size_t Bytes = Index.size() * sizeof(std::pair);
+  return Bytes / (1000 * 1000);

sammccall wrote:
> access to Index needs to be guarded by mutex
`Index.getMemorySize()` is a better estimate.



Comment at: clang-tools-extra/clangd/index/MemIndex.h:43
 private:
+  /// Returns estimate size of the index in megabytes.
+  size_t estimateMemoryUsage();

Can you return bytes here, and do lossy conversions at display time instead?

(Not sure the conversions are even worth it. e.g. it's may be interesting to 
compare even <1mb indexes)



Comment at: clang-tools-extra/clangd/index/MemIndex.h:44
+  /// Returns estimate size of the index in megabytes.
+  size_t estimateMemoryUsage();
+

I'd suggest making this part of the public interface, and implementing it for 
all (the implementations should be trivial).

This is useful e.g. for monitoring. We may want vim's :YcmDebugInfo to return 
this, etc.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:178
+size_t DexIndex::estimateMemoryUsage() {
+  size_t Bytes = LookupTable.size() * sizeof(std::pair);
+  Bytes += SymbolQuality.size() * sizeof(std::pair);

again, LookupTable.getMemorySize() and others.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:180
+  Bytes += SymbolQuality.size() * sizeof(std::pair);
+  Bytes += InvertedIndex.size() * sizeof(Token);
+  {

I think you're not counting the size of the actual symbols.
This is difficult to do precisely, but avoiding it seems misleading (we have 
"shared ownership" but it's basically exclusive). What's the plan here?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:182
+  {
+std::lock_guard Lock(Mutex);
+

why is only the InvertedIndex access guarded by the mutex?


https://reviews.llvm.org/D51154



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

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Sam McCall via Phabricator via cfe-commits]

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

This part looks good (I think everything controversial got moved into the other 
patch). Thanks!




Comment at: clangd/Threading.cpp:75
+std::string ThreadName;
+decltype(Action) ThreadFunc;
+decltype(CleanupTask) Cleanup;

nit: maybe call these CallAction and CallCleanupTask or something to make the 
parallels completely explicit. Up to you


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

first few comments, sorry I'll need to come back to this.




Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:90
   virtual DocID peek() const = 0;
   /// Retrieves boosting score. Query tree root should pass Root->peek() to 
this
   /// function, the parameter is needed to propagate through the tree. Given ID

Update documentation.

```Informs the iterator that the current document was consumed, and returns its 
boost.```

The rest of the doc seems a bit to far in the details, it's hard to follow.
Maybe just
```If this iterator has any child iterators that contain the document, consume()
should be called on those and their boosts incorporated.
consume() must *not* be called on children that don't contain the current 
doc.```



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:96
   /// shouldn't apply any boosting to the consumed item.
   virtual float consume(DocID ID) = 0;
 

There's two possible formulations here:
 - the DocID parameter is used by the Iterator to determine whether to actually 
decrement its limit (do we actually have that doc?)
 - the DocID parameter is redundant, as the parent should only call consume() 
on children that have the doc.
We need to be explicit about which. It seems you want the second (based on 
offline discussion). In this case we should either assert that the DocID 
matches peek(), or just drop the DocID parameter and use peek() instead. (If 
this later means we need to optimize peek, so be it).



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:168
 
+/// Returns LIMIT iterator, which is exhausted as soon requested number of 
items
+/// is consumed from the root of query tree.

This comment is accurate, but maybe a bit too concise as what's going on here 
is a little subtle.
Maybe expand a little like

```
Returns LIMIT iterator, which yields up to N elements of its child iterator.
Elements only count towards the limit if they are part of the final result set.
Therefore the following iterator (AND (2) (LIMIT (1 2) 1)) yields (2), not ().
```


https://reviews.llvm.org/D51029



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

[PATCH] D46795: [clangd] Don't query index when completing inside classes

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/CodeComplete.cpp:810
+return false;
+  auto Scope = CC.getCXXScopeSpecifier();
+  if (!Scope)

we could use a high level comment here explaining what the rest of the function 
is about. e.g.

// We also avoid ClassName::bar (but allow namespace::bar).

or just take the "we only query the index" comment, hoist it a bit, and 
generalize it (to not assume we're in name:: context)



Comment at: clangd/CodeComplete.cpp:826
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+  // Note that Identifier can only appear in dependent scopes and they never
+  // refer to namespaces.

(nit: this seems like undue weight, and could just be a trailing line comment 
`// Unresolved inside a template`)



Comment at: unittests/clangd/CodeCompleteTests.cpp:829
+TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
+  // clang-format off
+  auto Completions = completions(R"cpp(

Can we avoid disabling clang-format here? I do find it useful, and it adds 
noise.
IIRC moving the `.items` into the `EXPECT_THAT` results in sensible formatting.



Comment at: unittests/clangd/CodeCompleteTests.cpp:837
+Foo::^)cpp",
+{func("::SomeNameInTheIndex")}).items;
+  // clang-format on

I think you should also include "Foo::SomeNameInTheIndex" in the index. At 
first glance this doesn't seem to actually test all the failure modes (though I 
think it tested the one we actually had)



Comment at: unittests/clangd/CodeCompleteTests.cpp:845
+
+TEST(CompletionTest, NoIndexCompletionsInsideDependentCode) {
+  {

I think one of these would be enough, but up to you


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46795



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

[PATCH] D45999: [clangd] Retrieve minimally formatted comment text in completion.

[Sam McCall via Phabricator via cfe-commits]

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

Looks good, nits as always and I think you want a new test case.




Comment at: clangd/CodeComplete.cpp:817
   Result.IncludeGlobals = true;
-  Result.IncludeBriefComments = IncludeBriefComments;
+  // We never include brief comments, they are slow and don't provide useful
+  // results for non-doxygen comments.

"include brief comments" --> "parse doxygen syntax"? Or if you prefer, "parse 
\brief comments"

Calling these "brief comments" is confusing as a natural reading is "a one-line 
comment above the function" or similar.



Comment at: clangd/CodeCompletionStrings.h:29
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > "active" is a bit confusing - at this level we just care which arg 
> > > > you're interested in, right?
> > > Is this typically a substring of getDocComment for the function? If so, 
> > > noting that would make this clearer.
> > This refers to the parameter that should be reported as active in 
> > `signatureHelp`.
> > Agree that it requires too much context to understand, changed to a more 
> > detailed description involving `CurrentArg` parameter.
> > 
> > 
> > Is this typically a substring of getDocComment for the function? If so, 
> > noting that would make this clearer.
> I mechanically translated the original source code without looking too much 
> on what it does :-)
> 
> Extracting a substring from the function doc would be the behavior I expect.
> However, we simply try to get doc comment for parameter in the same way we do 
> for functions (e.g. look for comment doc for parameter decl, without looking 
> at function decl in the first place)
> It means we'll always get the empty doc currently. At least, I couldn't make 
> the current code produce any docs for the parameter.
> 
> I would still keep this function, as it seems to be called in the right 
> places. But we definitely need to parse a function comment instead.
I think this function is easy to understand in isolation, and by naming the 
parameter `CurrentArg` one can only really understand it in terms of its 
callsite in `signatureHelp`. I'd name it `ArgIndex` or similar instead, but up 
to you.



Comment at: clangd/CodeCompletionStrings.h:29
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.

sammccall wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > "active" is a bit confusing - at this level we just care which arg 
> > > > > you're interested in, right?
> > > > Is this typically a substring of getDocComment for the function? If so, 
> > > > noting that would make this clearer.
> > > This refers to the parameter that should be reported as active in 
> > > `signatureHelp`.
> > > Agree that it requires too much context to understand, changed to a more 
> > > detailed description involving `CurrentArg` parameter.
> > > 
> > > 
> > > Is this typically a substring of getDocComment for the function? If so, 
> > > noting that would make this clearer.
> > I mechanically translated the original source code without looking too much 
> > on what it does :-)
> > 
> > Extracting a substring from the function doc would be the behavior I expect.
> > However, we simply try to get doc comment for parameter in the same way we 
> > do for functions (e.g. look for comment doc for parameter decl, without 
> > looking at function decl in the first place)
> > It means we'll always get the empty doc currently. At least, I couldn't 
> > make the current code produce any docs for the parameter.
> > 
> > I would still keep this function, as it seems to be called in the right 
> > places. But we definitely need to parse a function comment instead.
> I think this function is easy to understand in isolation, and by naming the 
> parameter `CurrentArg` one can only really understand it in terms of its 
> callsite in `signatureHelp`. I'd name it `ArgIndex` or similar instead, but 
> up to you.
Ah, thanks.
I'd suggest noting that in the comment (this currently looks for comments 
attached to the parameter itself, and doesn't extract them from function 
documentation). I think most people looking at this would be interested in that 
detail!

Looking at the doxygen docs, it seems like they want syntax like this to 
work:`int foo(int x /** Doc */)`, and I'd hope we could also get this to work 
at some point:
```int foo(
  // Doc
  int x
);```

Not that I see much code in that style, but maybe we should write more!



Comment at: unittests/clangd/CodeCompleteTests.cpp:246
   // Check documentation.
-  EXPECT_IFF(Opts.IncludeBriefComments, Results.items,
- Contains(IsDocumented()));

[PATCH] D46001: [CodeComplete] Expose helpers to get RawComment of completion result.

[Sam McCall via Phabricator via cfe-commits]

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

This is pretty small and seems unlikely to be controversial. I think it's OK to 
commit at this point, it's been a few weeks.




Comment at: include/clang/Sema/CodeCompleteConsumer.h:1092
+const CodeCompleteConsumer::OverloadCandidate &Result,
+unsigned CurrentArg);
+

as in the other patch, I think `ArgIndex` is a better name for this parameter, 
when considering this function in isolation


Repository:
  rC Clang

https://reviews.llvm.org/D46001



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

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/ClangdUnit.h:53
   std::vector Diags;
-  InclusionLocations IncLocations;
+  std::vector Inclusions;
 };

this might be a good place to explain *why* we collect these - processes like 
code completion will need #include information later, and their compile action 
skips the preamble range



Comment at: clangd/Headers.h:47
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Inclusions);

as Ilya suggested, I like the idea of passing a callback(Inclusion) here rather 
than a vector reference. It's much more obvious at the callsite what's going to 
happen (and doesn't force the use of the vector).
Also lifetime issues are still a little complicated, but easier to debug I 
think.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46675



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

[PATCH] D46524: [clangd] Extract scoring/ranking logic, and shave yaks.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 146584.
sammccall added a comment.

fix diffbase?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46524

Files:
  clangd/CMakeLists.txt
  clangd/CodeComplete.cpp
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/ClangdUnitTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/QualityTests.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -12,15 +12,13 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "TestTU.h"
 #include "XRefs.h"
-#include "gmock/gmock.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
-#include "clang/Frontend/CompilerInvocation.h"
-#include "clang/Frontend/PCHContainerOperations.h"
-#include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -39,34 +37,6 @@
   std::vector Diagnostics) override {}
 };
 
-// FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef MainCode, StringRef HeaderCode = "") {
-  auto HeaderPath = testPath("foo.h");
-  auto MainPath = testPath("foo.cpp");
-  llvm::IntrusiveRefCntPtr VFS(
-  new vfs::InMemoryFileSystem());
-  VFS->addFile(MainPath, 0, llvm::MemoryBuffer::getMemBuffer(MainCode));
-  VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
-  if (!HeaderCode.empty()) {
-std::vector args = {"-include", HeaderPath.c_str()};
-Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
-  }
-  auto CI = createInvocationFromCommandLine(Cmd);
-
-  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
-  auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
-  std::make_shared(), VFS);
-  assert(AST.hasValue());
-  return std::move(*AST);
-}
-
-std::unique_ptr buildIndex(StringRef MainCode,
-StringRef HeaderCode) {
-  auto AST = build(MainCode, HeaderCode);
-  return MemIndex::build(indexAST(&AST));
-}
-
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -117,7 +87,7 @@
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 EXPECT_THAT(findDocumentHighlights(AST, T.point()), HighlightsFrom(T))
 << Test;
   }
@@ -139,10 +109,12 @@
   void  $f1[[f1]]() {}
 )cpp");
 
-  auto Index = buildIndex(SymbolCpp.code(), SymbolHeader.code());
+  TestTU TU;
+  TU.Code = SymbolCpp.code();
+  TU.HeaderCode = SymbolHeader.code();
+  auto Index = TU.index();
   auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) {
-auto AST = build(/*MainCode=*/Main.code(),
- /*HeaderCode=*/"");
+auto AST = TestTU::withCode(Main.code()).build();
 return clangd::findDefinitions(AST, Main.point(), Index.get());
   };
 
@@ -329,7 +301,7 @@
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 std::vector> ExpectedLocations;
 for (const auto &R : T.ranges())
   ExpectedLocations.push_back(RangeIs(R));
@@ -661,7 +633,7 @@
 
   for (const OneTest &Test : Tests) {
 Annotations T(Test.Input);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 Hover H = getHover(AST, T.point());
 
 EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input;
Index: unittests/clangd/TestTU.h
===
--- /dev/null
+++ unittests/clangd/TestTU.h
@@ -0,0 +1,59 @@
+//===--- TestTU.h - Scratch source files for testing *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===-===//
+//
+// Many tests for indexing, code completion etc are most naturally expressed
+// using code examples.
+// TestTU lets test define these examples in a common way without dealing with
+// the mechanics of VFS and compiler interactions, and then easily grab the
+// AST, particular symbols, etc.
+//
+//===-===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTT

[PATCH] D46524: [clangd] Extract scoring/ranking logic, and shave yaks.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 146591.
sammccall marked an inline comment as done.
sammccall added a comment.

Doxygen


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46524

Files:
  clangd/CMakeLists.txt
  clangd/CodeComplete.cpp
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/ClangdUnitTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/QualityTests.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -12,15 +12,13 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "TestTU.h"
 #include "XRefs.h"
-#include "gmock/gmock.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
-#include "clang/Frontend/CompilerInvocation.h"
-#include "clang/Frontend/PCHContainerOperations.h"
-#include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -39,34 +37,6 @@
   std::vector Diagnostics) override {}
 };
 
-// FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef MainCode, StringRef HeaderCode = "") {
-  auto HeaderPath = testPath("foo.h");
-  auto MainPath = testPath("foo.cpp");
-  llvm::IntrusiveRefCntPtr VFS(
-  new vfs::InMemoryFileSystem());
-  VFS->addFile(MainPath, 0, llvm::MemoryBuffer::getMemBuffer(MainCode));
-  VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-  std::vector Cmd = {"clang", "-xc++", MainPath.c_str()};
-  if (!HeaderCode.empty()) {
-std::vector args = {"-include", HeaderPath.c_str()};
-Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
-  }
-  auto CI = createInvocationFromCommandLine(Cmd);
-
-  auto Buf = MemoryBuffer::getMemBuffer(MainCode);
-  auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
-  std::make_shared(), VFS);
-  assert(AST.hasValue());
-  return std::move(*AST);
-}
-
-std::unique_ptr buildIndex(StringRef MainCode,
-StringRef HeaderCode) {
-  auto AST = build(MainCode, HeaderCode);
-  return MemIndex::build(indexAST(&AST));
-}
-
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -117,7 +87,7 @@
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 EXPECT_THAT(findDocumentHighlights(AST, T.point()), HighlightsFrom(T))
 << Test;
   }
@@ -139,10 +109,12 @@
   void  $f1[[f1]]() {}
 )cpp");
 
-  auto Index = buildIndex(SymbolCpp.code(), SymbolHeader.code());
+  TestTU TU;
+  TU.Code = SymbolCpp.code();
+  TU.HeaderCode = SymbolHeader.code();
+  auto Index = TU.index();
   auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) {
-auto AST = build(/*MainCode=*/Main.code(),
- /*HeaderCode=*/"");
+auto AST = TestTU::withCode(Main.code()).build();
 return clangd::findDefinitions(AST, Main.point(), Index.get());
   };
 
@@ -329,7 +301,7 @@
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 std::vector> ExpectedLocations;
 for (const auto &R : T.ranges())
   ExpectedLocations.push_back(RangeIs(R));
@@ -661,7 +633,7 @@
 
   for (const OneTest &Test : Tests) {
 Annotations T(Test.Input);
-auto AST = build(T.code());
+auto AST = TestTU::withCode(T.code()).build();
 Hover H = getHover(AST, T.point());
 
 EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input;
Index: unittests/clangd/TestTU.h
===
--- /dev/null
+++ unittests/clangd/TestTU.h
@@ -0,0 +1,59 @@
+//===--- TestTU.h - Scratch source files for testing *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===-===//
+//
+// Many tests for indexing, code completion etc are most naturally expressed
+// using code examples.
+// TestTU lets test define these examples in a common way without dealing with
+// the mechanics of VFS and compiler interactions, and then easily grab the
+// AST, particular symbols, etc.
+//
+//===-===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
+#define LLVM_CL

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/CodeComplete.cpp:247
+// Calculates include path and insertion edit for a new header.
+class IncludeInserter {
+public:

Do you think it would make sense to move this class to Headers.h? This would 
reduce the non-completion related code in this large file.



Comment at: clangd/CodeComplete.cpp:261
+  // The header name is either a URI or a literal include.
+  Expected> insert(StringRef FileName,
+  StringRef DeclaringHeader,

If we wanted to avoid URI dependencies in Headers.h but move this class there, 
the Declaring/Inserted headers could be HeaderFile (possibly invalid). 
toHeaderFile would stay here.

This might be a neater separation of concerns anyway.



Comment at: clangd/CodeComplete.cpp:365
+if (Includes && !D->IncludeHeader.empty()) {
+  // Fallback to canonical header if declaration location is invalid.
+  llvm::StringRef DeclaringHeader =

This strikes me as overly defensive. We should never have an include header but 
no declaration location, and if the declaration header is invalid then the 
include inserter should take care of that, not this code (I think?)



Comment at: clangd/CodeComplete.cpp:744
   PrecompiledPreamble const *Preamble;
+  const std::vector &Inclusions;
   StringRef Contents;

PreambleInclusions?



Comment at: clangd/CodeComplete.cpp:977
+ SemaCCInput,
+ /*CallbackBeforeAction*/ [&](CompilerInstance &Clang) {
+   Clang.getPreprocessor().addPPCallbacks(

this callback is a bit confusing, particularly sequencing/lifetimes.
semaCodeComplete is local, could we simplify it by coupling a bit, passing an 
`IncludeInserter*` ("will be populated by the preprocessor") and moving this 
code (and the preamble-includes handling) inside semaCodeComplete?

This would be a bit more natural if we had 
`IncludeInsertion::addExisting(Insertion)` instead of 
`IncludeInsertion::setExisting(vector)`, I think.



Comment at: clangd/CodeComplete.cpp:1142
+  {FileName, Command, Preamble,
+   /*Inclusions*/ {}, Contents, Pos, std::move(VFS), std::move(PCHs)});
   return Result;

nit: `/*Inclusions=*/`



Comment at: clangd/CodeComplete.h:73
 PrecompiledPreamble const *Preamble,
+const std::vector &Inclusions,
 StringRef Contents, Position Pos,

should this be PreambleInclusions, or is it broader than that?



Comment at: clangd/Headers.h:57
+/// preprocessor and build configs.
+class IncludePaths {
+public:

ioeric wrote:
> sammccall wrote:
> > This name suggests a set or sequence of include path entries (`-I` flags).
> > Can we merge this with `IncludeInsertion` and call it `IncludeInserter` or 
> > similar?
> I think there are three pretty independent parts of include insertion:
> 1. URI/Verbatim to `HeaderFile` conversion (which lives in HeaderInsertion in 
> CodeComplete.cpp right now).
> 2. Calculate include paths based on existing inclusions and HeaderSearch.
> 3. Calculate include insertion edits using `tooling::HeaderIncludes`.
> 
> Initially, I grouped all these in one class (something like 
> `HeaderInsertion`) which returns an edit given the URIs in the symbol info, 
> but it turned out to be hard to test. For example, among all 3 components, 2 
> was the most interesting piece to test, while an integration test is probably 
> enough for 1 and 3, but we would need to create URIs (due to 1) and retrieve 
> include name from edits (due to 3) in order to test 2. Since each individual 
> piece seemed independent enough, I decided to keep them separated. I also 
> kept URI logic out of the header library so that it could potentially be 
> shared with other features that are not index-based include insertion, or be 
> moved out of clangd as a tooling library. 
> 
> In the new revision, I refactored the code according to your suggestions and 
> got rid of this class.
OK, SGTM



Comment at: unittests/clangd/HeadersTests.cpp:48
+IgnoringDiagConsumer IgnoreDiags;
+auto CI = clang::createInvocationFromCommandLine(
+Argv,

ioeric wrote:
> sammccall wrote:
> > Is there any way we can avoid creating another copy of this code?
> This was moved here. Headers library no longer needs compiler instance. I'm 
> not sure if there is an easier way to test `HeaderSearch` and `PPCallbacks` 
> without a compiler instance...
Moved from where?

You could consider adding inclusions to TestTU (sorry it's in D46524 which I 
thought had already landed, but should today)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497



_

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Sam McCall via Phabricator via cfe-commits]

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

Nice, ship it!




Comment at: clangd/CodeComplete.cpp:317
+if (Includes && !D->IncludeHeader.empty()) {
+  // Fallback to canonical header if declaration location is invalid.
+  auto Edit = [&]() -> Expected> {

is this comment still relevant here?



Comment at: clangd/CodeComplete.cpp:792
+if (!Style) {
+  log("Failed to get FormaStyle for file" + Input.FileName +
+  ". Fall back to use LLVM style. Error: " +

nit: FormatStyle 



Comment at: clangd/CodeComplete.cpp:803
+
Clang->getPreprocessor().addPPCallbacks(collectInclusionsInMainFileCallback(
+Clang->getSourceManager(), [&Includes](Inclusion Inc) {
+  Includes->get()->addExisting(std::move(Inc));

nit: includes is a pointer, capture by value?



Comment at: clangd/CodeComplete.cpp:962
   Output = runWithSema();
+  Includes.release(); // Make sure this doesn't out-live Clang.
   SPAN_ATTACH(Tracer, "sema_completion_kind",

This leaks. reset (or = nullptr)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497



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

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

@malaperle to expand on what Eric said, adding proto hacks with false positives 
and no way to turn them off is indeed not the way to go here!
There's probably going to be other places we want to filter symbols too, and it 
should probably be extensible/customizable in some way.
We don't yet have enough examples to know what the structure should be 
(something regex based, a code-plugin system based on `Registry`, or something 
in between), thus the simplest/least invasive option for now (it's important 
for our internal rollout to have *some* mitigation in place).

@ioeric can you add a comment near the proto-filtering stuff indicating we 
should work out how to make this extensible?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D46751#1099633, @malaperle wrote:

> In https://reviews.llvm.org/D46751#1099235, @sammccall wrote:
>
> > @malaperle to expand on what Eric said, adding proto hacks with false 
> > positives and no way to turn them off is indeed not the way to go here!
> >  There's probably going to be other places we want to filter symbols too, 
> > and it should probably be extensible/customizable in some way.
> >  We don't yet have enough examples to know what the structure should be 
> > (something regex based, a code-plugin system based on `Registry`, or 
> > something in between), thus the simplest/least invasive option for now 
> > (it's important for our internal rollout to have *some* mitigation in 
> > place).
> >  @ioeric can you add a comment near the proto-filtering stuff indicating we 
> > should work out how to make this extensible?
>
>
> I agree with all of that. What I don't quite understand is why a flag is not 
> ok? Just a fail-safe switch in the mean time? You can even leave it on by 
> default so your internal service is not affected.


I think a flag doesn't solve much of the problem, and adds new ones:

- users have to find the flag, and work out how to turn it on in their editor, 
and (other than embedders) few will bother. And each flag hurts the usability 
of all the other flags.
- if this heuristic is usable only sometimes, that's at codebase granularity, 
not user granularity. Flags don't work that way. (Static index currently has 
this problem...)
- these flags end up in config files, so if we later remove the flag we'll 
*completely* break clangd for such users

> We know for a fact that some code bases like Houdini won't work with this, at 
> least there will be an option to make it work.

Is this still the case after the last revision (with the comment check?)
Agree we should only hardwire this on if we are confident that false positives 
are vanishingly small.




Comment at: clangd/index/SymbolCollector.cpp:101
+// we check whether it starts with PROTO_HEADER_COMMENT.
+bool isPrivateProtoSymbol(const NamedDecl &ND) {
+  const auto &SM = ND.getASTContext().getSourceManager();

We're going to end up calling this code on every decl/def we see.
Am I being paranoid by thinking we should check whether the file is a proto 
once, rather than doing a bunch of string matching every time?



Comment at: clangd/index/SymbolCollector.cpp:112
+
+  auto Name = ND.getName();
+  if (!Name.contains('_'))

this asserts if the name is not a simple identifier (Maybe operators or 
something will trigger this?).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

[PATCH] D46524: [clangd] Extract scoring/ranking logic, and shave yaks.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332378: [clangd] Extract scoring/ranking logic, and shave 
yaks. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46524?vs=146591&id=146881#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46524

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/Quality.cpp
  clang-tools-extra/trunk/clangd/Quality.h
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
  clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
  clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
  clang-tools-extra/trunk/unittests/clangd/TestTU.h
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -20,6 +20,7 @@
 #include "FuzzyMatch.h"
 #include "Headers.h"
 #include "Logger.h"
+#include "Quality.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "URI.h"
@@ -34,6 +35,9 @@
 #include "llvm/Support/Format.h"
 #include 
 
+// We log detailed candidate here if you run with -debug-only=codecomplete.
+#define DEBUG_TYPE "codecomplete"
+
 namespace clang {
 namespace clangd {
 namespace {
@@ -192,35 +196,6 @@
   return Result;
 }
 
-// Produces an integer that sorts in the same order as F.
-// That is: a < b <==> encodeFloat(a) < encodeFloat(b).
-uint32_t encodeFloat(float F) {
-  static_assert(std::numeric_limits::is_iec559, "");
-  static_assert(sizeof(float) == sizeof(uint32_t), "");
-  constexpr uint32_t TopBit = ~(~uint32_t{0} >> 1);
-
-  // Get the bits of the float. Endianness is the same as for integers.
-  uint32_t U;
-  memcpy(&U, &F, sizeof(float));
-  // IEEE 754 floats compare like sign-magnitude integers.
-  if (U & TopBit)// Negative float.
-return 0 - U;// Map onto the low half of integers, order reversed.
-  return U + TopBit; // Positive floats map onto the high half of integers.
-}
-
-// Returns a string that sorts in the same order as (-Score, Name), for LSP.
-std::string sortText(float Score, llvm::StringRef Name) {
-  // We convert -Score to an integer, and hex-encode for readability.
-  // Example: [0.5, "foo"] -> "4100foo"
-  std::string S;
-  llvm::raw_string_ostream OS(S);
-  write_hex(OS, encodeFloat(-Score), llvm::HexPrintStyle::Lower,
-/*Width=*/2 * sizeof(Score));
-  OS << Name;
-  OS.flush();
-  return S;
-}
-
 /// Creates a `HeaderFile` from \p Header which can be either a URI or a literal
 /// include.
 static llvm::Expected toHeaderFile(StringRef Header,
@@ -251,33 +226,6 @@
   const CodeCompletionResult *SemaResult = nullptr;
   const Symbol *IndexResult = nullptr;
 
-  // Computes the "symbol quality" score for this completion. Higher is better.
-  float score() const {
-float Score = 1;
-if (IndexResult)
-  Score *= quality(*IndexResult);
-if (SemaResult) {
-  // For now we just use the Sema priority, mapping it onto a 0-2 interval.
-  // That makes 1 neutral-ish, so we don't reward/penalize non-Sema results.
-  // Priority 80 is a really bad score.
-  Score *= 2 - std::min(80, SemaResult->Priority) / 40;
-
-  switch (static_cast(SemaResult->Availability)) {
-  case CXAvailability_Available:
-// No penalty.
-break;
-  case CXAvailability_Deprecated:
-Score *= 0.1f;
-break;
-  case CXAvailability_NotAccessible:
-  case CXAvailability_NotAvailable:
-Score = 0;
-break;
-  }
-}
-return Score;
-  }
-
   // Builds an LSP completion item.
   CompletionItem build(StringRef FileName, const CompletionItemScores &Scores,
const CodeCompleteOptions &Opts,
@@ -346,6 +294,7 @@
 return I;
   }
 };
+using ScoredCandidate = std::pair;
 
 // Determine the symbol ID for a Sema code completion result, if possible.
 llvm::Optional getSymbolID(const CodeCompletionResult &R) {
@@ -552,50 +501,12 @@
   UniqueFunction ResultsCallback;
 };
 
-// Tracks a bounded number of candidates with the best scores.
-class TopN {
-public:
-  using value_type = std::pair;
-  static constexpr size_t Unbounded = std::numeric_limits::max();
-
-  TopN(size_t N) : N(N) {}
-
-  // Adds a candidate to the set.
-  // Returns true if a candidate was dropped to get back under N.
-  bool push(value_type &&V) {
-bool Dropped = false;
-if (Heap.size() >= N) {
-  Dropped = true;
-  if (N > 0 && greater(V, Heap.front())) {
-std::pop_heap(Heap.begin(), Heap.end(), greater);
-Heap.b

[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/AST.h:32
+/// Returns true iff all redecls of \p D are in the main file.
+bool allDeclsInMainFile(const Decl *D);
 

Do you expect this to be reused?
If so, unit test? Otherwise this seems small enough to move where it's used.



Comment at: clangd/Quality.cpp:24
+  if (SemaCCResult.Declaration)
+AllDeclsInMainFile = allDeclsInMainFile(SemaCCResult.Declaration);
   if (SemaCCResult.Availability == CXAvailability_Deprecated)

ioeric wrote:
> Could you explain why `AllDeclsInMainFile` is necessary? I think it might 
> still be fine to boost score for symbols with a declaration in the main file 
> even if it has redecls in other files (e.g. function fwd in headers).
Agree. I think the better signal is (any) decl in main file.



Comment at: unittests/clangd/CodeCompleteTests.cpp:965
 
+TEST(CompletionTest, BoostCurrentFileDecls) {
+  MockFSProvider FS;

This should really be unit tested in QualityTests. I think instead rather than 
in addition - I'd like to get away from code completion ranking tests for every 
quality signal, it's really indirect and hard to maintain/review.



Comment at: unittests/clangd/CodeCompleteTests.cpp:966
+TEST(CompletionTest, BoostCurrentFileDecls) {
+  MockFSProvider FS;
+  FS.Files[testPath("foo.h")] = R"cpp(

(in QualityTests, we should be able to continue using TestTU)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943



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

[PATCH] D47187: [clangd] Skip .inc headers when canonicalizing header #include.

[Sam McCall via Phabricator via cfe-commits]

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

n




Comment at: clangd/index/CanonicalIncludes.cpp:50
+  if (I == Headers.end())
+return Headers[0]; // Fallback to the defining header.
+  llvm::StringRef Header = *I;

nit: here and elsewhere, you probably want "declaring" rather than defining



Comment at: clangd/index/CanonicalIncludes.h:52
+  /// Returns the canonical include for symbol with \p QualifiedName. \p 
Headers
+  /// is a stack of #includes that starts from the last included header (i.e.
+  /// header that defines the symbol) to the header file that is directly

This might be clearer slightly more concretely:
```
\p Headers is the include stack: Headers.front() is the file declaring the 
symbol, and Headers.back() is the main file```
(or "included by the main file", but having it be main-file might be neater)



Comment at: clangd/index/CanonicalIncludes.h:54
+  /// header that defines the symbol) to the header file that is directly
+  /// included by the main file. When mapping headers, this skips files that 
are
+  /// not supposed to be #included directly e.g. .inc file.

Nit: "this" is a bit ambiguous here, the parameter or the algorithm?
Consider just dropping this new part of the comment, it's kind of an 
implementation detail (hiding behind the word "canonical")



Comment at: clangd/index/SymbolCollector.cpp:209
+  while (true) {
+if (!Loc.isValid() || SM.isInMainFile(Loc))
+  break;

(as above, maybe want to include the main file for simplicity/symmetry)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47187



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

[PATCH] D47257: [clang-format] Break template declarations followed by comments

[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 for chasing this, good detective work!
Changes look plausible and tests are nice, so LG assuming you know what you're 
doing. If you're unsure, Manuel will give you better advice than me :)


Repository:
  rC Clang

https://reviews.llvm.org/D47257



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

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(Haven't reviewed the code yet, just design stuff)
I'm tempted to push back on the idea that we need to store any - "if it's fast 
enough for code complete, it's fast enough for GTD". But that's probably 
oversimplifying - we don't force reuse of a stable preamble for GTD I think. So 
we probably do want some cache.

In https://reviews.llvm.org/D47063#1104495, @malaperle wrote:

> In https://reviews.llvm.org/D47063#1104417, @ilya-biryukov wrote:
>
> > Maybe we can even store enough information to not need the AST for 
> > navigation at all and build it only for features like refactorings.
> >  @sammccall, let me know what are your thoughts on all of this.
>
>
> That's what I was thinking. I think I also had similar discussion with 
> @arphaman. I think storing a limited number of ASTs is a good interim 
> solution.


Agree - it's a good idea but we shouldn't block on it. A general-purpose xrefs 
index may solve this problem (and others) but requires a bunch of design. A 
narrower structure risks building a bunch of complexity for one feature we 
can't reuse for the next.

> Another alternative that I've considered was evicting the ASTs from memory 
> after a certain period of time, e.g. after 30 seconds of inactivity for some 
> file.

We discussed this a bit more offline. A time-based limit reduces idle RAM 
usage, and a weight limit (or just count) reduces peak RAM.
Relatively speaking, idle seems to be more important to our hosted/multiplexed 
use cases, and peak is more important when running on a developer machine.
Weight is probably easier to tune. Time is easier to implement as the TUs are 
independent.

So this gives us some options (assuming we want some caching, but not 
unlimited):

- Evict if old - this helps hosted a lot, and is simple to implement.
- Evict if full (this patch) - this helps standalone, and is more complex.
- Evict if full AND old - this can be tuned nicely for hosted and standalone. 
Most complex, but only a little more than the previous option.
- Evict if full OR old - this puts a hard limit on resource usage and controls 
idle, but doesn't seem useful for hosted as it can't drive idle to zero.

I think the main design decision is whether the cache logic requires TUs to 
interact (vs simple time eviction). If we pay that complexity cost we get a lot 
of flexibility to tweak the cache. It's the hosted stuff that's driving this 
(for Google) right now, but maybe that's just because we're not really 
measuring impact on workstations yet :)
So I think I like this policy as a starting point, but we should plan to bolt 
on time-based-expiry in the near future.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063



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

[PATCH] D47187: [clangd] Skip .inc headers when canonicalizing header #include.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/CanonicalIncludes.cpp:54
+  StringRef Header = *I;
+  if (!isLiteralInclude(Header)) {
+// If Header is not expected be included (e.g. .cc file), we fall back to

headers are paths, not , right?



Comment at: clangd/index/CanonicalIncludes.cpp:55
+  if (!isLiteralInclude(Header)) {
+// If Header is not expected be included (e.g. .cc file), we fall back to
+// the declaring header.

This logic could be merged with the .inc logic, right?
"Find the first header such that the extension is not '.inc', and isn't a 
recognized non-header file"



Comment at: clangd/index/CanonicalIncludes.cpp:60
+if (!Ext.empty() && !driver::types::onlyPrecompileType(
+driver::types::lookupTypeForExtension(Ext)))
+  return Headers[0];

lookupTypeForExtension can return TY_INVALID, which you have to check for 
(onlyPrecompileType will assert).

I think we should be conservative and accept TY_INVALID as possible headers - 
our goal here really is to exclude "obvious" non-headers like cpp.

(in this case you can probably drop the explicit empty check)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47187



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

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Having taken a closer look, I think the cache can be simplified/separated a bit 
more cleanly by returning shared pointers and not allowing lookups, instead 
restoring limited ownership in CppFile...

Happy to discuss more, esp if you might disagree :)




Comment at: clangd/ClangdUnit.h:132
 
-/// Manages resources, required by clangd. Allows to rebuild file with new
-/// contents, and provides AST and Preamble for it.
-class CppFile {
+/// A helper class that handles building preambles and ASTs for a file. Also
+/// adds some logging.

This may be change aversion, but I'm not sure this class does enough after this 
change - it doesn't store the inputs or the outputs/cache, so it kind of seems 
like it wants to be a function.

I guess the motivation here is that storing the outputs means dealing with the 
cache, and the cache is local to TUScheduler.
But `CppFile` is only used in TUScheduler, so we could move this there too? It 
feels like expanding the scope more than I'd like.

The upside is that I think it's a more natural division of responsibility: 
`CppFile` could continue to be the "main" holder of the `shared_ptr` 
(which we don't limit, but share), and instead of `Optional` it'd 
have a `weak_ptr` which is controlled and can be refreshed through 
the cache.

As discussed offline, the cache could look something like:
```
class Cache {
   shared_ptr put(ParsedAST);
   void hintUsed(ParsedAST*); // optional, bumps LRU when client reads
   void hintUnused(ParsedAST*); // optional, releases when client abandons
}

shared_ptr CppFile::getAST() {
  shared_ptr AST = WeakAST.lock();
  if (AST)
Cache.hintUsed(AST.get());
  else
WeakAST = AST = Cache.put(build...);
  return AST;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063



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

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: lib/Index/USRGeneration.cpp:691
+case BuiltinType::ULongAccum:
+  llvm_unreachable("No USR name mangling for fixed point types.");
 case BuiltinType::Float16:

leonardchan wrote:
> rsmith wrote:
> > leonardchan wrote:
> > > phosek wrote:
> > > > We need some solution for fixed point types.
> > > Added character ~ to indicate fixed point type followed by string 
> > > detailing the type. I have not added a test to it because logically, I do 
> > > not think we will ever reach that point. This logic is implemented in the 
> > > VisitType method, which mostly gets called by visitors to c++ nodes like 
> > > VisitTemplateParameterList, but we have disabled the use of fixed point 
> > > types in c++. VisitType does get called in VisitFunctionDecl but the 
> > > function exits early since we are not reading c++ (line 
> > > lib/Index/USRGeneration.cpp:238).
> > @rjmccall Is this an acceptable USR encoding? (Is our USR encoding scheme 
> > documented anywhere?)
> I chatted with @sammccall about this who said it was ok to add these types if 
> no one opposed this. I posted this on cfe-dev also and it seemed that no one 
> spoke up about it, so I thought this was ok.
> 
> I also couldn't find any standard or documentation about reserving characters 
> for USR. It doesn't seem that USR is also parsed in any way, so I don't think 
> I'm breaking anything (running ninja check-all passes).
> 
> And unless we also do enable this for C++, this code actually may not be run 
> since this method will not be visited if limited to C.
If the conclusion is to only allow these types for C, then you could punt on 
this by setting IgnoreResult so USR generation fails. As you say USRs only 
include type information in C++.

I think nothing parses them except humans, and using a prefix character is 
probably better than eating 24 single characters. But one prefix character + 
one trailing character might be less surprising.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



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

[PATCH] D47274: [clangd] Serve comments for headers decls from dynamic index only

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/CodeCompletionStrings.h:46
const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex);
+   unsigned ArgIndex, bool NoCommentsFromHeaders = true);
 

this invites double negation.
Also it doesn't seem like a great default, violates principle of least surprise.

prefer bool CommentsFromHeaders = true, or with no default.
(or even consider an enum)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47274



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

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks a lot better!
There do seem to be a lot of little classes that exist exactly one-per-TU 
(ASTWorker, ASTBuilder, CachedAST, to a lesser extent ParsedAST) and I'm not 
sure the balance of responsibilities is quite right. Some comments below.




Comment at: clangd/ClangdUnit.h:132
 
-/// Manages resources, required by clangd. Allows to rebuild file with new
-/// contents, and provides AST and Preamble for it.
-class CppFile {
+/// A helper class that handles building preambles and ASTs for a file. Also
+/// adds some logging.

ilya-biryukov wrote:
> sammccall wrote:
> > This may be change aversion, but I'm not sure this class does enough after 
> > this change - it doesn't store the inputs or the outputs/cache, so it kind 
> > of seems like it wants to be a function.
> > 
> > I guess the motivation here is that storing the outputs means dealing with 
> > the cache, and the cache is local to TUScheduler.
> > But `CppFile` is only used in TUScheduler, so we could move this there too? 
> > It feels like expanding the scope more than I'd like.
> > 
> > The upside is that I think it's a more natural division of responsibility: 
> > `CppFile` could continue to be the "main" holder of the 
> > `shared_ptr` (which we don't limit, but share), and instead of 
> > `Optional` it'd have a `weak_ptr` which is controlled 
> > and can be refreshed through the cache.
> > 
> > As discussed offline, the cache could look something like:
> > ```
> > class Cache {
> >shared_ptr put(ParsedAST);
> >void hintUsed(ParsedAST*); // optional, bumps LRU when client reads
> >void hintUnused(ParsedAST*); // optional, releases when client abandons
> > }
> > 
> > shared_ptr CppFile::getAST() {
> >   shared_ptr AST = WeakAST.lock();
> >   if (AST)
> > Cache.hintUsed(AST.get());
> >   else
> > WeakAST = AST = Cache.put(build...);
> >   return AST;
> > }
> > ```
> I've reimplemented the cache with weak_ptr/shared_ptr. 
> With a slightly different interface to hide more stuff in the cache API. 
> Please take a look and let me know what you think.
> 
> Nevertheless, I still find `CppFile` refactoring useful. It unties preambles 
> from the ASTs and I believe this is a good thing, given that their lifetimes 
> are different.
I'm not sure how much they were tangled before, they were computed in the same 
place, but accessed separately, and it's not sure you ever *want* to compute 
them separately? (possibly in unit tests?)

I do think making ASTWorker maintain the old preamble and pass it in is 
confusing. The remaining members are trivial and unrelated enough that I do 
think if the references to the preamble/ast are removed, then moving the 
remaining members to ASTWorker or to a dumb struct, and making these free 
functions would make it easier to navigate the class structure.



Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function()> ComputeF);

I think I understand this more as "updates the value" but the value is lazy...

I find this API somewhat hard to follow, maybe just because it's unfamiliar.
I've mostly seen cache APIs look like one of:
1. `Cache(function Compute)`, `Value Cache::get(Input)`
2. `void Cache::put(Key, Value)`, `Optional Cache::get(Key)`
3. `Handle Cache::put(Value)`, `Optional Handle::get()`

This one is `Slot Cache::create()`, `void Slot::update(function 
LazyV)`, `Value Slot::get()`.

It's similar-ish to 3, but has 3 nontrivial operations instead of 2, and the 
slot object is externally mutable instead of immutable, so it seems more 
complex. What does it buy us in exchange?

(1 & 2 work well with a natural key or inputs that are easy to compare, which 
we don't particularly have)



Comment at: clangd/TUScheduler.cpp:91
+/// Provides an LRU cache of ASTs.
+class TUScheduler::IdleASTs {
+  friend class CachedAST;

naming: `IdleASTs` doesn't mention the function of this class, which is a cache.
I'd suggest swapping the names: call the class `ASTCache` and the *instance* 
`IdleASTs` as that reflects its role within the TUScheduler.

For `CachedAST`, I think the relationship would be well-exposed by nesting it 
as `ASTCache::Entry`. This also gives you the `friend` for free, which seems 
like a hint that it's an appropriate structure.
(Though I'm not sure CachedAST is that useful)



Comment at: clangd/TUScheduler.cpp:153
+  /// one.
+  std::vector>>>
+  LRU; /* GUARDED_BY(Mut) */

as discussed offline, using a CachedAST* as a key shouldn't be necessary, the 
ParsedAST* should be enough I think.



Comment at: clangd/TUScheduler.cpp:157
+
+CachedAST::~CachedAST() { Owner.remove(*this); }
+

document why



Comment at: clangd/TUScheduler.h:48
+/// kept in memory.
+struct ASTRetentionParams {
+  /// Maximum numbe

[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

[Sam McCall via Phabricator via cfe-commits]

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

Nice, simple and will admit refinements later.

Just test nits and a trivial organizational thing.




Comment at: clangd/Quality.cpp:22
 
+namespace {
+bool hasDeclInMainFile(const Decl &D) {

nit: per coding style use static for functions
(I'm not sure it's a great rule, but since the ns only has this function...)



Comment at: clangd/Quality.h:52
   unsigned References = 0;
+  float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval.
 

this belongs in `SymbolRelevanceSignals` rather than this struct. In principle, 
SymbolQualitySignals should all be stuff we can store in a global index (which 
is the point of the split). I should probably add a comment to that effect :-)

You could be a little more specific on the semantics, e.g. "Proximity between 
the best declaration and the query location. [0-1] score where 1 is closest."



Comment at: unittests/clangd/QualityTests.cpp:96
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals Default;

please add a test for proximity here



Comment at: unittests/clangd/QualityTests.cpp:121
 
+TEST(QualityTests, BoostCurrentFileDecls) {
+  TestTU Test;

consider merging into SymbolRelevanceSignalsExtraction test, which tests the 
same entrypoint.

If not, move up next to that one.



Comment at: unittests/clangd/QualityTests.cpp:129
+  Test.Code = R"cpp(
+#include "foo.h"
+int ::test_func_in_header_and_cpp() {

this is not needed, the `#include` is implicit in TestTU

(and so you don't need to specify HeaderFilename either)



Comment at: unittests/clangd/QualityTests.cpp:155
+
+  /// Check the boost in proximity translates into a better score.
+  EXPECT_LE(FuncInHeader.evaluate(), FuncInCpp.evaluate());

this tests end-to-end, but the other tests verify input -> signals and signal 
-> score separately.

I'd prefer to keep (only) doing that, for consistency and because it's 
important we know/assert precisely what each half does so we can actually debug.



Comment at: unittests/clangd/TestTU.cpp:80
   continue;
-if (Result) {
+if (Result && ND->getCanonicalDecl() != Result) {
   ADD_FAILURE() << "Multiple Decls named " << QName;

well, I definitely wanted to flag this as an error (for the tests where this 
function was introduced).

Actually I think this is wrong for your test anyway - don't you want to test 
that no matter which decl the code completion result refers to, you get the 
same boost?

I'd suggest adding a `findDecls()` function that returns a vector, 
and implementing `findDecl()` in terms of it. In the 
`test_func_in_header_and_cpp` test, you could loop over `findDecls()` and run 
the assertions each time.

(I guess findDecls() should assert that the returned vector is non-empty? 
Slightly weird but might catch accidentally vacuous tests)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943



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

[PATCH] D47331: [clangd] Remove accessors for top-level decls from preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Wow, nice!
Just unsure about the test helper. (We can rewrite it in another way if needed)




Comment at: clangd/ClangdUnit.h:88
 
-  /// This function returns all top-level decls, including those that come
-  /// from Preamble. Decls, coming from Preamble, have to be deserialized, so
-  /// this call might be expensive.
-  ArrayRef getTopLevelDecls();
+  /// This function returns top-level decls, present in the main file of the
+  /// AST. The result does not include the decls that come from the preamble.

nit: remove comma



Comment at: unittests/clangd/TestTU.cpp:77
   const NamedDecl *Result = nullptr;
-  for (const Decl *D : AST.getTopLevelDecls()) {
+  for (const Decl *D : AST.getLocalTopLevelDecls()) {
 auto *ND = dyn_cast(D);

isn't this incorrect? Or should be "findDeclInMainFile" or similar?

I'd think this would conflict with your other patch, which uses this to test 
the boost of things that come from the header.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47331



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

[PATCH] D47223: [clangd] Handle enumerators in named, unscoped enums similarly to scoped enums

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D47223#1110571, @malaperle wrote:

> In https://reviews.llvm.org/D47223#1109247, @ilya-biryukov wrote:
>
> > I'm not sure if we have tests for that, but I remember that we kept the 
> > enumerators in the outer scope so that completion could find them..
> >  Am I right that this patch will change the behavior and we won't get 
> > enumerators in the following example:
> >
> >   /// foo.h
> >   enum Foo {
> > A, B, C
> >   };
> >  
> >   /// foo.cpp
> >   #include "foo.h"
> >  
> >   int a = ^ // <-- A, B, C should be in completion list here.
> >
>
>
> Not quite but still potentially problematic. With the patch, A, B, C would be 
> found but not ::A, ::B, ::C.


I don't understand how this would still work (at least when using the index). 
The index call would have Scopes set to the enclosing scopes {""}, which is 
equivalent to a search for ::A. Maybe I'm missing something.

In https://reviews.llvm.org/D47223#1112118, @ioeric wrote:

> I think the fundamental problem here is that C++ symbols (esp. enum 
> constants) can have different names (e.g. ns::Foo::A and ns::A).


Yup. There are a few other instances of this:

1. decls aliased with using decls - we model these as duplicate symbols
2. decls aliased with using directives - we ask clang to resolve the namespaces 
in scope when searching
3. inline namespaces - we mostly get away with ignoring them

AFAICS none of these are ideal for enums. (2 fails because the list of 
namespaces would get too long).
This is messy :-(


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47223



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

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The cache looks way simpler now, thank you!
As discussed offline, flattening ASTBuilder right into ASTWorker still seems 
like a good idea to me, but happy with what you come up with there.




Comment at: clangd/TUScheduler.cpp:71
+
+  /// Update the function used to compute the value.
+  void update(std::function()> ComputeF);

ilya-biryukov wrote:
> sammccall wrote:
> > I think I understand this more as "updates the value" but the value is 
> > lazy...
> > 
> > I find this API somewhat hard to follow, maybe just because it's unfamiliar.
> > I've mostly seen cache APIs look like one of:
> > 1. `Cache(function Compute)`, `Value Cache::get(Input)`
> > 2. `void Cache::put(Key, Value)`, `Optional Cache::get(Key)`
> > 3. `Handle Cache::put(Value)`, `Optional Handle::get()`
> > 
> > This one is `Slot Cache::create()`, `void Slot::update(function 
> > LazyV)`, `Value Slot::get()`.
> > 
> > It's similar-ish to 3, but has 3 nontrivial operations instead of 2, and 
> > the slot object is externally mutable instead of immutable, so it seems 
> > more complex. What does it buy us in exchange?
> > 
> > (1 & 2 work well with a natural key or inputs that are easy to compare, 
> > which we don't particularly have)
> As discussed offline, now we have a simpler version that keeps `unique_ptr`s 
> to idle ASTs and the clients are responsible for building the ASTs.
> Note that it's not a "cache" per se, so we might want a different name for 
> that.
> @sammccall, you suggested to call it a pool, I find it reasonable.  Should we 
> name it `ASTPool` instead of `ASTCache`?
I think the name is actually fine, it's still mostly a cache.
It does have things in common with a pool, but unrelated consumers can't share 
a resource, so I think that name is at least as misleading.



Comment at: clangd/TUScheduler.cpp:66
+
+/// Provides an LRU cache of ASTs.
+class TUScheduler::ASTCache {

I'd say a little more about the interaction here. e.g. 
```
/// An LRU cache of idle ASTs.
/// Because we want to limit the overall number of these we retain, the cache
/// owns ASTs (and may evict them) while their workers are idle.
/// Workers borrow them when active, and return them when done.



Comment at: clangd/TUScheduler.cpp:84
+  /// Store the value in the pool, possibly removing the last used AST.
+  void put(Key K, std::unique_ptr V) {
+std::unique_lock Lock(Mut);

consider assert(findByKey(K) == LRU.end()) as a precondition



Comment at: clangd/TUScheduler.cpp:92
+LRU.pop_back();
+// AST destructor may need to run, make sure it happens outside the lock.
+Lock.unlock();

Just "run the expensive destructor outside the lock"?
the "may not" case seems unimportant and slightly confusing here



Comment at: clangd/TUScheduler.cpp:94
+Lock.unlock();
+ForCleanup.reset();
+  }

this line isn't actually needed right?



Comment at: clangd/TUScheduler.cpp:342
+if (!AST)
+  return Action(llvm::make_error(
+  "invalid AST", llvm::errc::invalid_argument));

This failure doesn't get cached, correct? That's bad for performance.

But if we think this is always a clangd bug, it's probably fine. (and certainly 
simplifies things)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063



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

[PATCH] D44954: [clangd] Add "member" symbols to the index

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: hokein.
sammccall added inline comments.



Comment at: clangd/index/Index.h:158
   unsigned References = 0;
-
+  /// Whether or not this is symbol is meant to be used for the global
+  /// completion.

ioeric wrote:
> s/this is symbol/this symbol/?
Sorry, I hadn't seen this patch until now.
When it was part of the `workspace/symbol` patch, I was the other person 
concerned this was too coupled to existing use cases and preferred something 
more descriptive.

I dug up an analysis @hokein and I worked on. One concluseion we came to was 
that we thought results needed by `completion` were a subset of what 
`workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc and 
fragile though.

The cases we looked at were:

||private|member|local|primary template|template specialization|nested in 
template|
| code complete |N|N|N|Y|N|N|
| workspace/symbol |Y|Y|N|Y|Y|Y|
| go to defn |Y|Y|?|?|?|?|
(Y = index should return it, N = index should not return it, ? = don't care)

So the most relevant information seems to be:
 - is this private (private member, internal linkage, no decl outside cpp file, 
etc)
 - is this nested in a class type (or template)
 - is this a template specialization

I could live with bundling these into a single property (though they seem like 
good ranking signals, and we'd lose them for that purpose), it will certainly 
make a posting-list based index more efficient.

In that case I think we should have canonical documentation *somewhere* about 
exactly what this subset is, and this field should reference that.
e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and 
`IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too many 
different meanings - here we mean "index-based").



Comment at: clangd/index/Index.h:279
+  /// considered.
+  bool GlobalCodeCompletionOnly = false;
 };

please also avoid "global" here, e.g. `RestrictForCodeCompletion`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

[PATCH] D47466: [clangd] Avoid inserting new #include when declaration is present in the main file.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/CodeComplete.cpp:249
+  if (SemaResult->Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *D = SemaResult->getDeclaration()) {
+  const auto &SM = D->getASTContext().getSourceManager();

do you need to iterate over all redecls?



Comment at: clangd/CodeComplete.cpp:251
+  const auto &SM = D->getASTContext().getSourceManager();
+  ShouldInsertInclude =
+  !SM.isInMainFile(SM.getExpansionLoc(D->getLocStart()));

`ShouldInsertInclude = ShouldInsertInclude && ...` would be a little more 
future-proof


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47466



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

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Impl looks good.
Is there a way we can reasonably test this? (specifically that we don't just 
store all the ASTs forever)




Comment at: clangd/ClangdUnit.h:143
+std::shared_ptr
+buildPreamble(PathRef FileName, CompilerInvocation &CI,
+  std::shared_ptr OldPreamble,

nit: i think filename here is only used for logging, just use 
Inputs.CompileCommand.Filename?



Comment at: clangd/TUScheduler.cpp:103
+
+  /// Returns the cached value for \p K, or llvm::None if the value is not in
+  /// the cache anymore.

please also explicitly mention what nullptr means (even though that's really up 
to the caller of put()). None vs nullptr leaves room for confusion.



Comment at: clangd/TUScheduler.h:66
+  std::chrono::steady_clock::duration UpdateDebounce,
+  ASTRetentionPolicy RetentionPolicy = {});
   ~TUScheduler();

does this actually have more than one caller? what's the plan for exposing this 
option to embedders/CLI users (not saying we necessarily need the latter)?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063



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

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a subscriber: jkorous.

Hey Simon,

Sorry this patch got stalled. I hope you're still interested!
I agree there's space for providing some high-signal information about error 
conditions to power users/potential developers/administrators, even if it's not 
"polished" for end-users.

What are the semantics we want for these log levels? My guess is we need a 
third one:

1. it seems clear we want a level that indicates likely problems, with minimal 
noise. You want this, and this is useful for monitoring production/cloud issues 
too.
2. it may be useful to add context like LSP method names and "rebuilding 
preamble" to understand the context of errors and crashes. I think this is what 
Ilya was pushing for.
3. we also want to log details that allow you to see what's going on with only 
the log to go on. This is stuff like full LSP messages. I think it's important 
that we can do this with a production build.
4. finer-grained debugging things can probably be done in a targeted way using 
`LLVM_DEBUG` (CodeComplete has some of this for debugging scoring signals)

What do you think about 1=`elog()` (e for Error), 2=`log()`, 3=`vlog()` (v for 
Verbose), and using LLVM_DEBUG for 4?

I don't have a strong opinion of what the default should be long term, but 
either 1 with `-v` and `-vv` to change,  or 2 with `-q` and `-v`, seem 
reasonable...

(Internally at google we use something like glog 
 which calls these levels ERROR, INFO, VLOG(1) 
and VLOG(2), with INFO as default - this convention explains some of our 
confusion over the semantics of vlog!)

If you don't think we need a third level, or don't have time for it now, I'm 
pretty happy with this patch as it is but would suggest rename of `log`,`vlog` 
--> `elog`,`log` to make it clearer that the former is for problems only.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226



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

[PATCH] D38414: [clangd] simplify ClangdLSPServer by private-inheriting callback interfaces. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.

There doesn't seem to be any real separation between the current three objects.
Feel free to reject this if you find the current style valuable, though.

(Mostly I'm just looking around for cleanups to help me understand the code).


https://reviews.llvm.org/D38414

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h

Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -14,6 +14,7 @@
 #include "GlobalCompilationDatabase.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "ProtocolHandlers.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 
@@ -24,7 +25,7 @@
 
 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
-class ClangdLSPServer {
+class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
   bool SnippetCompletions,
@@ -37,18 +38,35 @@
   void run(std::istream &In);
 
 private:
-  class LSPProtocolCallbacks;
-  class LSPDiagnosticsConsumer : public DiagnosticsConsumer {
-  public:
-LSPDiagnosticsConsumer(ClangdLSPServer &Server);
-
-virtual void
-onDiagnosticsReady(PathRef File,
-   Tagged> Diagnostics);
-
-  private:
-ClangdLSPServer &Server;
-  };
+  // DiagnosticsConsumer
+  virtual void
+  onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override;
+
+  // ProtocolCallbacks
+  void onInitialize(StringRef ID, InitializeParams IP,
+JSONOutput &Out) override;
+  void onShutdown(JSONOutput &Out) override;
+  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
+ JSONOutput &Out) override;
+  void onDocumentDidChange(DidChangeTextDocumentParams Params,
+   JSONOutput &Out) override;
+  void onDocumentDidClose(DidCloseTextDocumentParams Params,
+  JSONOutput &Out) override;
+  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
+  StringRef ID, JSONOutput &Out) override;
+  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
+ StringRef ID, JSONOutput &Out) override;
+  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onCodeAction(CodeActionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput &Out) override;
 
   std::vector
   getFixIts(StringRef File, const clangd::Diagnostic &D);
@@ -74,7 +92,6 @@
   // Various ClangdServer parameters go here. It's important they're created
   // before ClangdServer.
   DirectoryBasedGlobalCompilationDatabase CDB;
-  LSPDiagnosticsConsumer DiagConsumer;
   RealFileSystemProvider FSProvider;
 
   // Server must be the last member of the class to allow its destructor to exit
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -9,7 +9,6 @@
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
-#include "ProtocolHandlers.h"
 
 using namespace clang::clangd;
 using namespace clang;
@@ -38,50 +37,8 @@
 
 } // namespace
 
-ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
-ClangdLSPServer &Server)
-: Server(Server) {}
-
-void ClangdLSPServer::LSPDiagnosticsConsumer::onDiagnosticsReady(
-PathRef File, Tagged> Diagnostics) {
-  Server.consumeDiagnostics(File, Diagnostics.Value);
-}
-
-class ClangdLSPServer::LSPProtocolCallbacks : public ProtocolCallbacks {
-public:
-  LSPProtocolCallbacks(ClangdLSPServer &LangServer) : LangServer(LangServer) {}
-
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
-  void onShutdown(JSONOutput &Out) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput &Out) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput &Out) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput &Out) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput &Out) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
-

[PATCH] D38414: [clangd] simplify ClangdLSPServer by private-inheriting callback interfaces. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 117256.
sammccall added a comment.

- Address review comments


https://reviews.llvm.org/D38414

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h

Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -14,6 +14,7 @@
 #include "GlobalCompilationDatabase.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "ProtocolHandlers.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 
@@ -24,7 +25,7 @@
 
 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
-class ClangdLSPServer {
+class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
   bool SnippetCompletions,
@@ -37,18 +38,35 @@
   void run(std::istream &In);
 
 private:
-  class LSPProtocolCallbacks;
-  class LSPDiagnosticsConsumer : public DiagnosticsConsumer {
-  public:
-LSPDiagnosticsConsumer(ClangdLSPServer &Server);
-
-virtual void
-onDiagnosticsReady(PathRef File,
-   Tagged> Diagnostics);
-
-  private:
-ClangdLSPServer &Server;
-  };
+  // Implement DiagnosticsConsumer.
+  virtual void
+  onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override;
+
+  // Implement ProtocolCallbacks.
+  void onInitialize(StringRef ID, InitializeParams IP,
+JSONOutput &Out) override;
+  void onShutdown(JSONOutput &Out) override;
+  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
+ JSONOutput &Out) override;
+  void onDocumentDidChange(DidChangeTextDocumentParams Params,
+   JSONOutput &Out) override;
+  void onDocumentDidClose(DidCloseTextDocumentParams Params,
+  JSONOutput &Out) override;
+  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
+  StringRef ID, JSONOutput &Out) override;
+  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
+ StringRef ID, JSONOutput &Out) override;
+  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onCodeAction(CodeActionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
+JSONOutput &Out) override;
+  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput &Out) override;
 
   std::vector
   getFixIts(StringRef File, const clangd::Diagnostic &D);
@@ -74,7 +92,6 @@
   // Various ClangdServer parameters go here. It's important they're created
   // before ClangdServer.
   DirectoryBasedGlobalCompilationDatabase CDB;
-  LSPDiagnosticsConsumer DiagConsumer;
   RealFileSystemProvider FSProvider;
 
   // Server must be the last member of the class to allow its destructor to exit
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -9,7 +9,6 @@
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
-#include "ProtocolHandlers.h"
 
 using namespace clang::clangd;
 using namespace clang;
@@ -38,50 +37,8 @@
 
 } // namespace
 
-ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
-ClangdLSPServer &Server)
-: Server(Server) {}
-
-void ClangdLSPServer::LSPDiagnosticsConsumer::onDiagnosticsReady(
-PathRef File, Tagged> Diagnostics) {
-  Server.consumeDiagnostics(File, Diagnostics.Value);
-}
-
-class ClangdLSPServer::LSPProtocolCallbacks : public ProtocolCallbacks {
-public:
-  LSPProtocolCallbacks(ClangdLSPServer &LangServer) : LangServer(LangServer) {}
-
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
-  void onShutdown(JSONOutput &Out) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput &Out) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput &Out) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput &Out) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput &Out) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput &Out) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef 

[PATCH] D38414: [clangd] simplify ClangdLSPServer by private-inheriting callback interfaces. NFC

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314587: [clangd] simplify ClangdLSPServer by 
private-inheriting callback interfaces. NFC (authored by sammccall).

Repository:
  rL LLVM

https://reviews.llvm.org/D38414

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -9,7 +9,6 @@
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
-#include "ProtocolHandlers.h"
 
 using namespace clang::clangd;
 using namespace clang;
@@ -38,50 +37,8 @@
 
 } // namespace
 
-ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
-ClangdLSPServer &Server)
-: Server(Server) {}
-
-void ClangdLSPServer::LSPDiagnosticsConsumer::onDiagnosticsReady(
-PathRef File, Tagged> Diagnostics) {
-  Server.consumeDiagnostics(File, Diagnostics.Value);
-}
-
-class ClangdLSPServer::LSPProtocolCallbacks : public ProtocolCallbacks {
-public:
-  LSPProtocolCallbacks(ClangdLSPServer &LangServer) : LangServer(LangServer) {}
-
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
-  void onShutdown(JSONOutput &Out) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput &Out) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput &Out) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput &Out) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput &Out) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput &Out) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput &Out) override;  
-
-private:
-  ClangdLSPServer &LangServer;
-};
-
-void ClangdLSPServer::LSPProtocolCallbacks::onInitialize(StringRef ID,
- InitializeParams IP,
- JSONOutput &Out) {
+void ClangdLSPServer::onInitialize(StringRef ID, InitializeParams IP,
+   JSONOutput &Out) {
   Out.writeMessage(
   R"({"jsonrpc":"2.0","id":)" + ID +
   R"(,"result":{"capabilities":{
@@ -94,79 +51,74 @@
   "definitionProvider": true
 }}})");
   if (IP.rootUri && !IP.rootUri->file.empty())
-LangServer.Server.setRootPath(IP.rootUri->file);
+Server.setRootPath(IP.rootUri->file);
   else if (IP.rootPath && !IP.rootPath->empty())
-LangServer.Server.setRootPath(*IP.rootPath);
+Server.setRootPath(*IP.rootPath);
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onShutdown(JSONOutput &Out) {
-  LangServer.IsDone = true;
-}
+void ClangdLSPServer::onShutdown(JSONOutput &Out) { IsDone = true; }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidOpen(
-DidOpenTextDocumentParams Params, JSONOutput &Out) {
+void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams Params,
+JSONOutput &Out) {
   if (Params.metadata && !Params.metadata->extraFlags.empty())
-LangServer.CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
-std::move(Params.metadata->extraFlags));
-  LangServer.Server.addDocument(Params.textDocument.uri.file,
-Params.textDocument.text);
+CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
+ std::move(Params.metadata->extraFlags));
+  Server.addDocument(Params.textDocument.uri.file, Params.textDocument.text);
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidChange(
-DidChangeTextDocumentParams Params, JSONOutput &Out) {
+void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams Params,
+  JSONOutput &Out) {
   // We only support full syncing right now.
-  LangServer.Server.addDocument(Params.textDocument.uri.file,
-Params.contentChanges[0]

[PATCH] D37973: [clang-format] Fix regression about short functions after #else

[Sam McCall via Phabricator via cfe-commits]

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

This test looks like it was intended to catch some case, maybe we're now 
mishandling some case like

  if (foo)
  {
  }
  else
  {
// this can now be merged
  }

But there's no testcase and I'm guessing, so let's fix the `#else` bug :-)


https://reviews.llvm.org/D37973



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

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.

Make the ProtocolHandlers glue between JSONRPCDispatcher and
ClangdLSPServer generic.
Eliminate small differences between methods, de-emphasize the unimportant
distinction between notifications and methods.

ClangdLSPServer is no longer responsible for producing a complete
JSON-RPC response, just the JSON of the result object. (In future, we
should move that JSON serialization out, too).
Handler methods now take a context object that we may hang more
functionality off in the future.

Added documentation to ProtocolHandlers.


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 117374.
sammccall added a comment.

- clang-format


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 117835.
sammccall marked 5 inline comments as done.
sammccall added a comment.

- ClangLSPServer.h should refer to ShutdownParams, not NoParams
- Address review comments


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: clangd/Protocol

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! All addressed (except removing ShutdownParams, which I'm not sure is 
worthwhile).




Comment at: clangd/ClangdLSPServer.h:47
   // Implement ProtocolCallbacks.
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput &Out) override;
-  void onShutdown(JSONOutput &Out) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput &Out) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput &Out) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput &Out) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput &Out) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput &Out) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput &Out) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput &Out) override;
+  void onInitialize(Ctx &C, InitializeParams &Params) override;
+  void onShutdown(Ctx &C, NoParams &Params) override;

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Maybe there's a way to have a typed return value instead of `Ctx`?
> > This would give an interface that's harder to misuse: one can't forget to 
> > call `reply` or call it twice.
> > 
> > Here's on design that comes to mind.
> > Notification handlers could have `void` return, normal requests can return 
> > `Optional` or `Optional` (with result json).
> > `Optional` is be used to indicate whether error occurred while processing 
> > the request.
> > 
> After putting more thought into it, `Ctx`-based API seems to have an 
> advantage: it will allow us to easily implement async responses.
> E.g., we can run code completion on a background thread and continue 
> processing other requests. When completion is ready, we will simply call 
> `Ctx.reply` to return results to the language client.
> 
> To make that possible, can we allow moving `RequestContext` and pass it 
> by-value instead of by-ref?
Yeah I thought about returning a value... it certainly reads more nicely, but I 
don't think we're ready to do a good job in this patch:
 - return value should be an object ready to be serialized (rather than a JSON 
string) - I don't want to bring that in scope here, but it might affect the 
details of the API
 - there's several cases we know about (return object, no reply, error reply) 
and some we're not sure about (async as you mention - any multiple responses)? 
I think this needs some design, and I don't yet know the project well enough to 
drive it.

I've switched to passing Ctx by value as you suggest (though it's certainly 
cheap enough to copy, too).



Comment at: clangd/ClangdLSPServer.h:48
+  void onInitialize(Ctx &C, InitializeParams &Params) override;
+  void onShutdown(Ctx &C, NoParams &Params) override;
+  void onDocumentDidOpen(Ctx &C, DidOpenTextDocumentParams &Params) override;

ilya-biryukov wrote:
> Maybe there's a way to get rid of `NoParams`?
> E.g. by adding a overload to `HandlerRegisterer`?
Even if there was, I think I prefer the regularity (changed this to 
ShutdownParams - oops!).

Otherwise the signature's dependent on some combination of {current LSP, 
whether we actually implement the options, whether we've defined any 
extensions}. It's harder to remember, means changing lots of places when these 
factors change, and complicates the generic code.

Maybe I've just spent too long around Stubby, though - WDYT?


https://reviews.llvm.org/D38464



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

[PATCH] D38627: [clangd] Added move-only function helpers.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/Function.h:9
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUNCTION_H

Maybe add a file comment "provides analogues to std::function that supports 
move semantics"?



Comment at: clangd/Function.h:36
+  template 
+  UniqueFunction(Callable Func)
+  : CallablePtr(llvm::make_unique<

Do you want this constructor to be explicit?

If not, I think you should be able to simplify the callsites in ClangdServer.h



Comment at: clangd/Function.h:77
+/// `operator()` can only be called once, as some of the arguments could be
+/// std::move'ed into the callable on first call.
+template  struct ForwardBinder {

nit: just 'moved'? std::move is just a cast...



Comment at: clangd/Function.h:117
+
+/// Creates an object that stores a callable (\p F) and first arguments to the
+/// callable (\p As) and allows to call \p F with \Args at a later point.

I find these "first arg" APIs a bit awkward, and can lead to writing confusing 
APIs for easier binding. Not sure there's a good alternative, though.



Comment at: clangd/Function.h:121
+///
+/// The returned object can only be called once, as \p As are std::forwarded'ed
+/// (therefore can be std::move`d) into \p F for the call.

nit: can -> must?


https://reviews.llvm.org/D38627



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

[PATCH] D38629: [clangd] Added a callback-based codeComplete in clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'm missing some context for this one:

- what's the goal? Make the code read more naturally, change the async model, 
or something else?
- what's the end state for codeComplete specifically? will we switch to the new 
overload and delete the other, or is makeFutureAPIFromCallback here to stay?


https://reviews.llvm.org/D38629



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

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 118782.
sammccall added a comment.

Rebase


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-watch-files.test
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: test/clangd/did-change-watch-files.test

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315577: [clangd] less boilerplate in RPC dispatch (authored 
by sammccall).

Repository:
  rL LLVM

https://reviews.llvm.org/D38464

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/test/clangd/did-change-watch-files.test
  clang-tools-extra/trunk/test/clangd/fixits.test

Index: clang-tools-extra/trunk/test/clangd/fixits.test
===
--- clang-tools-extra/trunk/test/clangd/fixits.test
+++ clang-tools-extra/trunk/test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "charact

[PATCH] D38629: [clangd] Added a callback-based codeComplete in clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/ClangdServer.cpp:51
 
+template 
+std::future makeFutureAPIFromCallback(

I'm not sure we need a template here rather than just writing the code directly.



Comment at: clangd/ClangdServer.cpp:257
+  // A task that will be run asynchronously.
+  auto Task =
+  // 'mutable' to reassign Preamble variable.

Isn't this exactly where you'd want to use ForwardBinder?
(Yeah, it looks like the work scheduler itself has this functionality too)



Comment at: clangd/ClangdServer.h:261
+  void codeComplete(
+  UniqueFunction>)> Callback,
+  PathRef File, Position Pos,

Hmm, generally having a callback as the last parameter is nicer, but this 
doesn't play nicely with optional params...


https://reviews.llvm.org/D38629



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

[PATCH] D38720: [clangd] Report proper kinds for 'Keyword' and 'Snippet' completion items.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Going to land this on Ilya's behalf as he's out on vacation.


https://reviews.llvm.org/D38720



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

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: test/clangd/authority-less-uri.test:33
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"jsonrpc":"2.0","id":3,"result":null}
+Content-Length: 33

Having the shutdown/exit boilerplate in every test seems unfortunate.
(I realize this isn't new here, but it's getting worse!)

Does clangd exit cleanly on stdin EOF? If it could be made to do so, maybe we 
could just have e.g. the initialize test do the full sequence, and the other 
tests omit shutdown.


https://reviews.llvm.org/D38939



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

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

So I'm not totally convinced that treating an abrupt exit or EOF as an error is 
worth the hassle (others might disagree), but it's certainly reasonable.
Could you add a test for the new behavior?
something like

RUN: not clangd -run-synchronously < %s


RUN: not clangd -run-synchronously < %s



https://reviews.llvm.org/D38939



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

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hi Alex! I'm working on clangd, but am pretty new to the project, so forgive 
some naive questions.

I'm a bit unclear at a high level what the new abstractions in this patch add, 
in terms of wiring refactorings up to clangd and other tools.
My understanding is: we have a hierarchy of refactorings consisting of actions 
(roughly: user intent?) and rules (roughly: implementations).

Clangd needs to be able to:

- enumerate the actions and their associated rules: possible via 
`createRefactoringActions()`
- find names for these (machine readable and human readable): possible at the 
`Action` level, not `Rule` yet
- determine whether rules can be applied and what configuration is needed: 
possible via the `Rule` API
- invoke rules: possible via the `Rule` API

So AFAICT, clangd could be hooked up to the existing Refactor API, without 
`EditorCommand` or `EditorClient`. What's the gain?


Repository:
  rL LLVM

https://reviews.llvm.org/D38985



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

[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
Herald added a subscriber: mgorny.

This lets you visualize clangd's activity on different threads over time,
and understand critical paths of requests and object lifetimes.
The data produced can be visualized in Chrome (at chrome://tracing), or
in a standalone copy of catapult (http://github.com/catapult-project/catapult)

This patch consists of:

- a command line flag "-trace" that causes clangd to emit JSON trace data
- an API (in Trace.h) allowing clangd code to easily add events to the stream
- several initial uses of this API to capture JSON-RPC requests, builds, logs

Example result: https://photos.app.goo.gl/12L9swaz5REGQ1rm1

Caveats:

- JSON serialization is ad-hoc (isn't it everywhere?) so the API is limited to 
naming events rather than attaching arbitrary metadata. I'd like to fix this (I 
think we could use a JSON-object abstraction).
- The recording is very naive: events are written immediately by locking a 
mutex. Contention on the mutex might disturb performance.
- For now it just traces instants or spans on the current thread. There are 
other things that make sense to show (cross-thread flows, non-thread resources 
such as ASTs). But we have to start somewhere.


https://reviews.llvm.org/D39086

Files:
  clangd/CMakeLists.txt
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/JSONRPCDispatcher.cpp
  clangd/ProtocolHandlers.cpp
  clangd/Trace.cpp
  clangd/Trace.h
  clangd/tool/ClangdMain.cpp
  test/clangd/trace.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/TraceTests.cpp

Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,116 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+  if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+  }
+  SmallString<32> S;
+  return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+bool VerifyObject(yaml::Node &N, std::map Expected) {
+  auto* M = dyn_cast(&N);
+  if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+  }
+  bool Match = true;
+  SmallString<32> Tmp;
+  for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue;
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+  ADD_FAILURE() << KS << " is not a string";
+  Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+  ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+  Match = false;
+}
+Expected.erase(I);
+  }
+  for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+  }
+  return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+  // Capture some events.
+  std::string JSON;
+  raw_string_ostream OS(JSON);
+  trace::start(OS);
+  {
+trace::Span S("A");
+trace::log("B");
+  }
+  trace::stop();
+
+  // Get the root JSON object using the YAML parser.
+  SourceMgr SM;
+  yaml::Stream Stream(JSON, SM);
+  auto Doc = Stream.begin();
+  ASSERT_NE(Doc, Stream.end());
+  auto* Root = dyn_cast_or_null(Doc->getRoot());
+  ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+  // We expect in order:
+  //   displayTimeUnit: "ns"
+  //   traceEvents: [process name, thread name, start span, log, end span]
+  // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+  auto Prop = Root->begin();
+  ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+  ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+  EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+  ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+  EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+  auto* Events = dyn_cast_or_null(Prop->getValue());
+  ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+  auto Event = Events->begin();
+  ASSERT_NE(Event, Events->end()) << "Expected process name";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+  ASSERT_NE(++

[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/JSONRPCDispatcher.cpp:230
 
-  // Finally, execute the action for this JSON message.
-  if (!Dispatcher.call(JSONRef, Out))
-Out.log("JSON dispatch failed!\n");
+  {
+// Finally, execute the action for this JSON message.

ioeric wrote:
> Why this empty block?
Oops, was tracing here but it didn't turn out to be useful. Reverted.



Comment at: clangd/Trace.cpp:90
+
+void start(raw_ostream &OS) {
+  assert(!T && "start() called twice without stop()");

ioeric wrote:
> Would it make sense to have a helper class which handles starting/stopping 
> in, say, constructor/destructor? ... so that we don't need to worry about 
> forgetting to stop?
I didn't do this for a couple of reasons:

 - it obscures the static-stateful nature of this code, making it look like 
e.g. you could dynamically turn on tracing while clangd is running. (Which 
would be cool, but wouldn't currently work)
 - it's some more boilerplate, and isn't actually very convenient to ClangdMain 
(since `start()` is conditional)



Comment at: clangd/tool/ClangdMain.cpp:98
+  llvm::errs() << "Error while opening trace file: " << EC.message();
+}
+trace::start(*TraceStream);

ioeric wrote:
> Is this intended? Start tracing even if there is an error?
Oops, thanks!


https://reviews.llvm.org/D39086



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

[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 119562.
sammccall marked 3 inline comments as done.
sammccall added a comment.

Address review comments.


https://reviews.llvm.org/D39086

Files:
  clangd/CMakeLists.txt
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/JSONRPCDispatcher.cpp
  clangd/ProtocolHandlers.cpp
  clangd/Trace.cpp
  clangd/Trace.h
  clangd/tool/ClangdMain.cpp
  test/clangd/trace.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/TraceTests.cpp

Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+  if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+  }
+  SmallString<32> S;
+  return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node &N, std::map Expected) {
+  auto* M = dyn_cast(&N);
+  if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+  }
+  bool Match = true;
+  SmallString<32> Tmp;
+  for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue;  // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+  ADD_FAILURE() << KS << " is not a string";
+  Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+  ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+  Match = false;
+}
+Expected.erase(I);
+  }
+  for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+  }
+  return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+  // Capture some events.
+  std::string JSON;
+  raw_string_ostream OS(JSON);
+  trace::start(OS);
+  {
+trace::Span S("A");
+trace::log("B");
+  }
+  trace::stop();
+
+  // Get the root JSON object using the YAML parser.
+  SourceMgr SM;
+  yaml::Stream Stream(JSON, SM);
+  auto Doc = Stream.begin();
+  ASSERT_NE(Doc, Stream.end());
+  auto* Root = dyn_cast_or_null(Doc->getRoot());
+  ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+  // We expect in order:
+  //   displayTimeUnit: "ns"
+  //   traceEvents: [process name, thread name, start span, log, end span]
+  // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+  auto Prop = Root->begin();
+  ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+  ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+  EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+  ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+  EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+  auto* Events = dyn_cast_or_null(Prop->getValue());
+  ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+  auto Event = Events->begin();
+  ASSERT_NE(Event, Events->end()) << "Expected process name";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected span start";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected log message";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected span end";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+  ASSERT_EQ(++Event, Events->end());
+  ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   ClangdTests.cpp
+  TraceTests.cpp
   )
 
 target_link_libraries(ClangdTests
Index: test/clangd

[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace

ilya-biryukov wrote:
> Maybe use `unique_ptr` (or even `llvm::Optional`)? Otherwise 
> we leak memory and don't flush the stream if users of tracing API forget to 
> call `stop()`. (I think I'm on the same page with @ioeric here  about 
> forgetting a call to `stop()`).
>  
> Having a static global of non-pod type is a no-go, but we could use static 
> local variables:
> 
> ```
> llvm::Optional& getTracerInstance() {
>  static llvm::Optional Tracer;
>  return Tracer;
> }
> ```
So Eric's solution of a scoped object that would be owned by main() makes sense 
to me, and I'm happy to add it (though personally not sure it carries its 
weight).

However I don't understand *your* suggestion :-)
> Having a static global of non-pod type is a no-go, but we could use static 
> local variables:
> llvm::Optional& getTracerInstance() {
>   static llvm::Optional Tracer;
>   return Tracer;
> }
So it's not global, which means we're not calling Optional() on startup, which 
would actually be totally safe (I guess the constructor should be marked as 
`constexpr`).

But we *are* calling `~Optional()` and therefore `~Tracer()` at shutdown. And 
maybe `~Tracer()` calls something that logs, and llvm::errs() has already been 
destroyed... *this* is why avoid static objects, right?

> Otherwise we leak memory and don't flush the stream if users of tracing API 
> forget to call stop()
The intended time to call `stop()` is at the end of the program. If you forget 
to call it, we won't be leaking memory :-) We indeed won't flush the stream, 
but that wouldn't be safe (see above).

The other option is the user is calling `start()` and `stop()` multiple times, 
and they call `start()` twice without stopping. We already assert on this, and 
anyone who doesn't reproducibly hit that assert is well into racy UB-land 
(logging methods may be called concurrently with each other, but everything 
else must be synchronized).

Overall I'm not really seeing what scenario we can detect and safely do 
something about.


https://reviews.llvm.org/D39086



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

[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 119864.
sammccall added a comment.

Address FIXME now that r316330 is in.


https://reviews.llvm.org/D39086

Files:
  clangd/CMakeLists.txt
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/JSONRPCDispatcher.cpp
  clangd/ProtocolHandlers.cpp
  clangd/Trace.cpp
  clangd/Trace.h
  clangd/tool/ClangdMain.cpp
  test/clangd/trace.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/TraceTests.cpp

Index: unittests/clangd/TraceTests.cpp
===
--- /dev/null
+++ unittests/clangd/TraceTests.cpp
@@ -0,0 +1,117 @@
+//===-- TraceTests.cpp - Tracing unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Trace.h"
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/YAMLParser.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using namespace llvm;
+
+MATCHER_P(StringNode, Val, "") {
+  if (arg->getType() != yaml::Node::NK_Scalar) {
+*result_listener << "is a " << arg->getVerbatimTag();
+return false;
+  }
+  SmallString<32> S;
+  return Val == static_cast(arg)->getValue(S);
+}
+
+// Checks that N is a Mapping (JS object) with the expected scalar properties.
+// The object must have all the Expected properties, but may have others.
+bool VerifyObject(yaml::Node &N, std::map Expected) {
+  auto* M = dyn_cast(&N);
+  if (!M) {
+ADD_FAILURE() << "Not an object";
+return false;
+  }
+  bool Match = true;
+  SmallString<32> Tmp;
+  for (auto Prop : *M) {
+auto* K = dyn_cast_or_null(Prop.getKey());
+if (!K) continue;
+std::string KS = K->getValue(Tmp).str();
+auto I = Expected.find(KS);
+if (I == Expected.end()) continue;  // Ignore properties with no assertion.
+
+auto* V = dyn_cast_or_null(Prop.getValue());
+if (!V) {
+  ADD_FAILURE() << KS << " is not a string";
+  Match = false;
+}
+std::string VS = V->getValue(Tmp).str();
+if (VS != I->second) {
+  ADD_FAILURE() << KS << " expected " << I->second << " but actual " << VS;
+  Match = false;
+}
+Expected.erase(I);
+  }
+  for (const auto& P : Expected) {
+ADD_FAILURE() << P.first << " missing, expected " << P.second;
+Match = false;
+  }
+  return Match;
+}
+
+TEST(TraceTest, SmokeTest) {
+  // Capture some events.
+  std::string JSON;
+  raw_string_ostream OS(JSON);
+  trace::start(OS);
+  {
+trace::Span S("A");
+trace::log("B");
+  }
+  trace::stop();
+
+  // Get the root JSON object using the YAML parser.
+  SourceMgr SM;
+  yaml::Stream Stream(JSON, SM);
+  auto Doc = Stream.begin();
+  ASSERT_NE(Doc, Stream.end());
+  auto* Root = dyn_cast_or_null(Doc->getRoot());
+  ASSERT_NE(Root, nullptr) << "Root should be an object";
+
+  // We expect in order:
+  //   displayTimeUnit: "ns"
+  //   traceEvents: [process name, thread name, start span, log, end span]
+  // (The order doesn't matter, but the YAML parser is awkward to use otherwise)
+  auto Prop = Root->begin();
+  ASSERT_NE(Prop, Root->end()) << "Expected displayTimeUnit property";
+  ASSERT_THAT(Prop->getKey(), StringNode("displayTimeUnit"));
+  EXPECT_THAT(Prop->getValue(), StringNode("ns"));
+  ASSERT_NE(++Prop, Root->end()) << "Expected traceEvents property";
+  EXPECT_THAT(Prop->getKey(), StringNode("traceEvents"));
+  auto* Events = dyn_cast_or_null(Prop->getValue());
+  ASSERT_NE(Events, nullptr) << "traceEvents should be an array";
+  auto Event = Events->begin();
+  ASSERT_NE(Event, Events->end()) << "Expected process name";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "process_name"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected thread name";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "M"}, {"name", "thread_name"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected span start";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "B"}, {"name", "A"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected log message";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "i"}, {"name", "B"}}));
+  ASSERT_NE(++Event, Events->end()) << "Expected span end";
+  EXPECT_TRUE(VerifyObject(*Event, {{"ph", "E"}}));
+  ASSERT_EQ(++Event, Events->end());
+  ASSERT_EQ(++Prop, Root->end());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   ClangdTests.cpp
+  TraceTests.cpp
   )
 
 target_link_libraries(ClangdTests
Index: test/clangd/trace.test
===

[PATCH] D39057: [clangd][WIP] Integrate the refactoring actions into clangd

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

As noted on the other patch, I'm not sure EditorCommands is a useful 
abstraction.

This shows up one of the problems: clangd is mostly decoupled from the actual 
behavior/semantics of the commands by the EditorCommands abstraction. However 
LSP doesn't say what the structure of ExecuteCommandParams.arguments is - it 
depends on the individual command. But clangd needs to parse those arguments 
out of the JSON! So we're left with all the EditorCommands needing to take the 
same set of arguments, which then get hardcoded in clangd.

Coupling clangd more closely to the refactorings seems simpler and more 
flexible - what would we be losing?

Other than the EditorCommands question, implementation looks fine!




Comment at: clangd/Protocol.h:535
+struct CommandArgument {
+  union {
+llvm::AlignedCharArrayUnion SelectionRange;

Why a union-of-unions instead of AlignedCharArrayUnion?


Repository:
  rL LLVM

https://reviews.llvm.org/D39057



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

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:123
+  size_t Bytes = Index.estimateMemoryUsage();
+  for (const auto &Scheme : URISchemes) {
+// std::string contains chars with sizeof(char) == 1.

ioeric wrote:
> I think the URI scheme names should be negligible.
yeah, just drop this I think.



Comment at: clang-tools-extra/clangd/index/Index.h:391
+  // FIXME(kbobyrev): Currently, this only returns the size of index itself
+  // excluding the size of actual symbol slab index refers to. It might be
+  // useful to return both.

it might be useful --> we should include both :-)


https://reviews.llvm.org/D51154



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

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+  float consume() override {
+if (reachedEnd())
   return DEFAULT_BOOST_SCORE;

this isn't possible, as the item being consumed is the value of peek().
You could assert !reachedEnd() or just delete this.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:227
+  float consume() override {
+if (reachedEnd())
   return DEFAULT_BOOST_SCORE;

and again here



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:341
+  float consume() override {
+if (!reachedEnd())
+  --ItemsLeft;

if condition is always true
you could replace the if with an assertion, but the child will assert, so 
better just to remove it.
replace if with assertion



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:348
+  llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+OS << "(LIMIT items left " << ItemsLeft << *Child << ")";
+return OS;

ISTM this should show the immutable state at the start of the query, rather 
than the mutated state?
Or if you really think it's useful, both.

e.g. (LIMIT 5(3) )



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:358
 
 std::vector> consume(Iterator &It, size_t Limit) {
   std::vector> Result;

I think you can remove the Limit parameter now, replacing it with an outer 
Limit iterator



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:362
It.advance(), ++Retrieved) {
 DocID Document = It.peek();
+Result.push_back(std::make_pair(Document, It.consume()));

you can inline this now



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:267
 
 TEST(DexIndexIterators, Limit) {
+  const PostingList L0 = {3, 6, 7, 20, 42, 100};

Is this testing the "limit" parameter to consume, or the limit iterator?
It shouldn't test both just because they happen to share a name :-)

(As noted above, I think you can remove the limit parameter)


https://reviews.llvm.org/D51029



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

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Nice fix!




Comment at: lib/Format/Format.cpp:1312
   auto &Line = *AnnotatedLines[i];
   if (Line.startsWith(tok::kw_namespace) ||
+  Line.startsWith(tok::kw_inline, tok::kw_namespace) ||

these 3 startswith checks appear 4 times now, you could pull out a helper 
function `startsWithNamespace` in FormatToken.h or something like that.
Up to you.


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: unittests/Format/FormatTest.cpp:7582
Style);
+  verifyFormat("export namespace Foo\n"
+   "{};",

you may want to add tests for other modules TS syntax (e.g. non-namespace 
export decls).
It seems this works well today, but without tests it could regress.


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly just reviewed the `references()` implementation.

For the infrastructure I have some high level questions:

- why are we combining SymbolSlab and occurrences?
- what's the motivation for the separate preamble/main-file indexes
- why put more functionality into SymbolCollector?

Let's discuss offline a bit if that works?




Comment at: clangd/TUScheduler.h:54
 
+class ParsingCallbacks {
+public:

(we're replacing a std::function, why not just a struct with two 
std::functions? saves boilerplate in a bunch of places...)



Comment at: clangd/XRefs.cpp:666
+std::vector references(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration,
+ const SymbolIndex *Index) {

I'm not sure unpacking the reference options into individual bools is going to 
scale well. I'd suggest passing the whole struct instead.



Comment at: clangd/XRefs.cpp:675
+  llvm::DenseSet IDs;
+  llvm::DenseSet NonLocalIDs;
+  for (const auto *D : Symbols.Decls) {

"local" is ambiguous here (this function cares both about file-local references 
and function-local symbols).
Consider `ExternallyVisibleIDs`



Comment at: clangd/XRefs.cpp:681
+   : "Non-local\n");
+  if (!clang::index::isFunctionLocalSymbol(D))
+NonLocalIDs.insert(*ID);

(This saves us hitting the index to look up references for one symbol, not sure 
if it's worth it at all).

I wouldn't particularly trust the helpers in clang::index::. Indeed the 
implementation looks wrong here (e.g. it would treat lambda captures as global, 
I think?)

I'd prefer D->getParentFunctionOrMethod() here.





Comment at: clangd/XRefs.cpp:688
+  SymbolOccurrenceKind::Reference | SymbolOccurrenceKind::Definition;
+  if (IncludeDeclaration)
+Filter |= SymbolOccurrenceKind::Declaration;

I'm not sure this is the best interpretation of LSP `includeDeclaration`.

The LSP spec is very vague here, and we can usually assume it's written with 
typescript in mind :-) where the declaration/definition distinction doesn't 
really exist.
It appears the distinction they're trying to draw is declaration vs "only" 
reference, rather than definition vs "only" declaration. So I think if we're 
going to respect this flag, we should *exclude* definitions when the flag is 
false.

Alternatively we could punt on this and just ignore the flag for now, and add 
it in a followup.



Comment at: clangd/XRefs.cpp:694
+
+  SymbolCollector Collector({nullptr, &Opts}, {});
+  index::IndexingOptions IndexOpts;

Reusing symbolcollector here seems odd.

It forces us to go through SymbolID rather than just using the Decl*. This is 
certainly reliable for global symbols, but I've got no idea how robust USRs are 
for local symbols. It also ends up building the Symbol objects, which we then 
throw away.

How much of SymbolCollector are we really reusing, vs a custom 
IndexDataConsumer? How is this different from document-highlights? Maybe we can 
reuse the consumer.



Comment at: clangd/XRefs.cpp:699
+  IndexOpts.IndexFunctionLocals = true;
+  // Only find references for the current main file.
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),

This comment is surprising as it stands. Maybe 
```// Look in the AST for references from the current file.```
(and hoist the commentto the top of the code that deals with AST, currently 
line 686)



Comment at: clangd/XRefs.cpp:713
+
+  // Query index for non-local symbols.
+  if (Index && !NonLocalIDs.empty()) {

This is also a bit confusing - the symbols are non-local (in the function-scope 
sense) and the references are non-local (in the file sense).
Consider:
```
// Consult the index for references in other files.
// We only need to consider symbols visible to other files.
```



Comment at: clangd/XRefs.cpp:722
+  if (auto LSPLoc = toLSPLocation(O.Location, "")) {
+if (!llvm::is_contained(SeenURIs, O.Location.FileURI))
+  Results.push_back(*LSPLoc);

in the usual case, SeenURIs is the current file (if there were any local 
usages), or empty (if there weren't).

This means if there are refs from the current file in the index but not locally 
(they were deleted), you'll return them when it would be better not to.

Why not just obtain the URI for the main file directly and compare that, 
instead of extracting it from the local references found?



Comment at: clangd/index/Index.h:368
   std::vector Symbols;  // Sorted by SymbolID to allow lookup.
-};
-
-// Describes the kind of a symbol occurrence.
-//
-// This is a bitfield which can be combined from different kinds.
-enum class Symb

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:34
+consumeIDs(std::unique_ptr It,
+   size_t Limit = std::numeric_limits::max()) {
+  auto Root = createLimit(move(It), Limit);

remove limit param here too?
Where you're *testing* limit, the tests will be clearer if you do it explicitly.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:279
   DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(4, 7, 8, 20, 42, 100));
+  EXPECT_THAT(consumeIDs(move(DocIterator)), ElementsAre(3, 6, 7, 20, 42, 
100));
 

this doesn't seem to test the limit iterator (at least not more than it's 
tested elsewhere)


https://reviews.llvm.org/D51029



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D50958#1212179, @ilya-biryukov wrote:

> Parts of this patch were landed as https://reviews.llvm.org/D50847 and 
> https://reviews.llvm.org/D50889, happy to discuss the design decisions 
> ("merged" dynamic index, two callbacks), but that's probably out of scope of 
> this patch.


Ah ok, that explains some of the unrelated changes.
I do have some questions there that would be good to discuss. Meanwhile, 
@hokein can you rebase this patch against head?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958



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

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/Index.h:310
+
+  llvm::ArrayRef findOccurrences(const SymbolID &ID) const {
+auto It = SymbolOccurrences.find(ID);

As discussed offline: the merge of occurrences into SymbolSlab seems 
problematic to me.
On the consumer side, we have a separation between Symbol APIs and 
SymbolOccurrence APIs - they don't really interact.
The Symbol type can often only be used with SymbolSlab, and so including 
occurrences drags them into the mess for consumers that don't care about them.

For producers (index implementations), they will usually have both and they may 
want to share arena storage. But this probably doesn't matter much, and if it 
does we can use another mechanism (like allowing SymbolSlabBuilder and 
SymbolOccurrenceSlab to share UniqueStringSaver)



Comment at: clangd/index/SymbolCollector.cpp:439
   SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 

note that here we've done basically all the work needed to record the 
occurrence.

If you add a DenseMap then you'll have 
enough info at the end to fill in the occurrences, like we do with 
referenceddecls -> references.



Comment at: clangd/index/SymbolCollector.h:59
+  // If none, all symbol occurrences will be collected.
+  llvm::Optional> IDs = llvm::None;
+};

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Could you elaborate on what this option will be used for? How do we know 
> > > in advance which symbols we're interested in?
> > This is used for finding references in the AST as a part of the xref 
> > implementation, basically the workflow would be:
> > 
> > 1. find SymbolIDs of the symbol under the cursor, using 
> > `DeclarationAndMacrosFinder`
> > 2. run symbol collector to find all occurrences in the main AST with all 
> > SymbolIDs in #1
> > 3. query the index, to get more occurrences
> > 4. merge them  
> Can we instead find all the occurences in  `DeclarationAndMacrosFinder` 
> directly?
> Extra run of `SymbolCollector` means another AST traversal, which is slow by 
> itself, and SymbolCollector s designed for a much more hairy problem, its 
> interface is just not nicely suited for things like only occurrences. The 
> latter seems to be a simpler problem, and we can have a simpler interface to 
> solve it (possibly shared between SymbolCollector and 
> DeclarationAndMacrosFinder). WDYT?
> 
> 
Yeah, I don't think we need this.
For "find references in the AST" we have an implementation in XRefs for 
highlights which we don't need to share.



Comment at: clangd/index/SymbolCollector.h:40
   struct Options {
+struct CollectSymbolOptions {
+  bool CollectIncludePath = false;

Not sure this split is justified.
if IDs goes away (see below), all that's left can be represented in a 
SymbolOccurenceKind filter (which is 0 to collect no occurrences)



Comment at: clangd/index/SymbolCollector.h:76
+// If not null, SymbolCollector will collect symbol occurrences.
+const CollectOccurrenceOptions *OccurrenceOpts;
   };

collecting symbols doesn't actually need to be optional I think - it's the core 
responsibility of this class, and "find occurrences of a decl in an ast" can be 
implemented more easily in other ways


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The description says "LSP clients", i.e. editors.
For editors (e.g. whether editor can handle snippets), LSP capability 
negotiation rather than flags is the right thing. I suspect this is true for 
including completion fixes.
For users (e.g. I like behavior X), flags are the right mechanism. But we don't 
need to expose every option, just the important ones.




Comment at: clangd/tool/ClangdMain.cpp:197
 
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",

ilya-biryukov wrote:
> I wonder if we should make the `IncludeFixIts` option hidden?
> It currently only works for our YCM integration, VSCode and other clients 
> break.
why would a user want to turn this on or off?



Comment at: clangd/tool/ClangdMain.cpp:198
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
+llvm::cl::desc(

ilya-biryukov wrote:
> This is a bit too generic name for the binary. Maybe `-completion-fixits`?
clang calls changes attached to diagnostics "fix-it hints", clangd calls them 
"fixes".

This isn't either of those, maybe it should have a different name?
Or if clang is going to consider them as another type of fixit, we should 
probably call them fixes.
Maybe `-completions-with-fixes`, which hints that if the flag is off you're 
going to lose these completions entirely?



Comment at: clangd/tool/ClangdMain.cpp:202
+"Like changing an arrow(->) member access to dot(.) member access."),
+llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+

ilya-biryukov wrote:
> Maybe specify the value here explicitly?
> The defaults for users of the clangd binary (what the option is for) is not 
> necessarily the best default for the `ClangdServer` clients (what the default 
> in the struct is for).
> More importantly, it's useful to be more straightforward about the defaults 
> we have for the options.
Not sure I agree here, this is consistent with the other options above. It's 
the other `ClangdServer` clients that are the weirdos, they should have to be 
explicit :-)

The defaults are clear when running with `-help`, which is how users will see 
them.



Comment at: clangd/tool/ClangdMain.cpp:205
+static llvm::cl::opt EnableFunctionArgSnippets(
+"enable-function-arg-snippets",
+llvm::cl::desc("Gives snippets for function arguments, when disabled only "

ilya-biryukov wrote:
> I wonder if we can infer this setting from the `-completion-style' 
> (`=bundled` implies `enable-function-arg-snippets == false`)
> @sammccall, WDYT?
They're not inextricably linked, the main connection is "these options both 
need good signaturehelp support to be really useful".
But we might want to link them just to cut down on number of options.

I don't have a strong opinion, I don't use `-completion-style=bundled`. Can we 
find a few people who do and ask if they like this setting?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214



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

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, javed.absar.

- DynamicIndex doesn't implement ParsingCallbacks, to make its role clearer. 
ParsingCallbacks is a separate object owned by the receiving TUScheduler. (I 
tried to get rid of the "index-like-object that doesn't implement index" but it 
was too messy).
- Clarified(?) docs around DynamicIndex - fewer details up front, more details 
inside.
- Exposed dynamic index from ClangdServer for memory monitoring and more direct 
testing of its contents (actual tests not added here, wanted to get this out 
for review)
- Removed a redundant and sligthly confusing filename param in a callback


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*Old

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 162401.
sammccall added a comment.

Add note about the overlap between preamble index across files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx,
-  std::shared_ptr PP) {
+  [&](ASTContext &Ctx, std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, &Ctx, std::move(PP));
+Index.update(FooCpp, &Ctx, std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
==

[PATCH] D50385: [clangd] Collect symbol occurrences in SymbolCollector

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty good!




Comment at: clangd/index/Index.h:376
+
+   llvm::ArrayRef find(const SymbolID &ID) const {
+ auto It = Occurrences.find(ID);

assert frozen?
looking up in a non-frozen array is probably a mistake.
if we choose to optimize this, it probably won't be possible.



Comment at: clangd/index/Index.h:377
+   llvm::ArrayRef find(const SymbolID &ID) const {
+ auto It = Occurrences.find(ID);
+ if (It == Occurrences.end())

return Occurrences.lookup(ID)?



Comment at: clangd/index/SymbolCollector.cpp:227
 
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+  SymbolOccurrenceKind Kind;

nit: toOccurrenceKind



Comment at: clangd/index/SymbolCollector.cpp:229
+  SymbolOccurrenceKind Kind;
+  for (auto Mask :
+   {SymbolOccurrenceKind::Declaration, SymbolOccurrenceKind::Definition,

If you want to filter out the unsupported bits, maybe just add an explicit 
`AllOccurrenceKinds` constant to the header file, and `return 
AllOccurrenceKinds & Roles` here? (plus casts)



Comment at: clangd/index/SymbolCollector.cpp:328
+  if ((static_cast(Opts.OccurrenceFilter) & Roles) &&
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+DeclOccurrences[ND].emplace_back(Loc, Roles);

just compute the spelling loc once and reuse?



Comment at: clangd/index/SymbolCollector.cpp:329
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+DeclOccurrences[ND].emplace_back(Loc, Roles);
+

you get the spelling loc on the previous line to check for mainfile - so surely 
we should be using spelling loc here?



Comment at: clangd/index/SymbolCollector.cpp:455
+
+  for (auto& It : DeclOccurrences) {
+if (auto ID = getSymbolID(It.first)) {

nit: const auto& for clarity since we're not mutating



Comment at: clangd/index/SymbolCollector.cpp:460
+  std::string FileURI;
+  if (auto DeclLoc = getTokenLocation(LocAndRole.first,
+  ASTCtx->getSourceManager(), Opts,

so this seems maybe  gratuitously inefficient, we're copying the filename then 
going through the URI conversion dance for each reference - even though the 
filename is the same for each.

consider splitting out part of `getTokenLocation` into 
`getTokenRange(SymbolLocation&)` and only calling that here.



Comment at: clangd/index/SymbolCollector.h:54
 // Populate the Symbol.References field.
 bool CountReferences = false;
 // Every symbol collected will be stamped with this origin.

this should be next to OccurrenceFilter, they're very closely related (the name 
mismatch is a little unfortunate)



Comment at: clangd/index/SymbolCollector.h:121
+  using DeclOccurrence = std::pair;
+  llvm::DenseMap> 
DeclOccurrences;
+  // All symbol occurrences collected from the AST, assembled on finish().

please move next to ReferencedDecls/ReferencedMacros so the comment applies to 
this too



Comment at: unittests/clangd/SymbolCollectorTests.cpp:466
+  SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+  testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("foo")));
+  EXPECT_THAT(

this is cute - if possible, consider adding a matcher factory function for 
readability here, so you can write `EXPECT_THAT(..., 
HaveRanges(Main.ranges("foo"))`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for finding this problem, this fix *mostly* looks good (though I think 
we can probably drop memoization).

I'm a bit uncomfortable about the places where we need the type, because this 
is the only thing forcing us to parse before we've picked a command to 
transfer, and the number of commands we need to parse is data-dependent and 
hard to reason about.

Let me think about this a little - I suspect slightly more invasive changes 
(change the concept of type, tweak the heuristics, or do a "lightweight parse" 
to get the type) might make this cleaner and performance more predictable.




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:440
   Result.emplace_back(std::move(Command));
-  if (Result.back().Type == types::TY_INVALID)
-Result.pop_back();

ilya-biryukov wrote:
> We can't look at 'Type' at this point anymore, because it needs parsing of 
> TranserableCommands. Not sure what's the best way to replace it. @sammccall, 
> any ideas?
> 
So filtering out this has a couple of effects
 - it's a performance optimization (don't bother indexing filenames for useless 
files). We don't need this
 - it prevents a TY_INVALID command being chosen for transfer. I'm not sure 
whether this would occur often enough to be a problem in practice.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:133
+assert(TraitsComputed && "calling transferTo on moved-from object");
+const CommandTraits &T = getTraits();
+CompileCommand Result = T.Cmd;

I think you're overthinking things with the memoization here (of course I say 
this as the person who underthought it!)

AIUI, the problem is that *eagerly* parsing all the compile commands takes 3x 
as long as reading them, which hurts startup time with big 
`compile_commands.json`.

But I think we can afford to just parse them when `transferTo` is called, 
without memoization. (Remember we only hit this code path when querying a file 
*not in the CDB*, so it should never get called in a tight loop).

The benefit of slightly reducing the latency of`getCompileCommand` for unknown 
files when we happen to pick the same template file for the second time... it's 
unclear to me, and the code would be easier to follow without it.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:159
+  llvm::StringRef filename() const {
+assert(TraitsComputed && "calling filename on moved-from object");
+return OriginalCmd.Filename;

Is this so important to dynamically check? Most types don't.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:171
+  // compute, so we only compute on first access.
+  struct CommandTraits {
+// Flags that should not apply to all files are stripped from CommandLine.

Traits is a bit vague, and a bit template-nightmare-y!
maybe ParsedCommand?




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:383
   S.Preferred = PreferredLanguage == types::TY_INVALID ||
-PreferredLanguage == Commands[S.Index].Type;
+PreferredLanguage == Commands[S.Index].type();
   S.Points = Candidate.second;

I think this is going to force parsing of all candidates that get any points at 
all, with a flat directory structure this could be quite a lot :-(



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:383
   S.Preferred = PreferredLanguage == types::TY_INVALID ||
-PreferredLanguage == Commands[S.Index].Type;
+PreferredLanguage == Commands[S.Index].type();
   S.Points = Candidate.second;

sammccall wrote:
> I think this is going to force parsing of all candidates that get any points 
> at all, with a flat directory structure this could be quite a lot :-(
ah, now I see that the memoization also allows us to pretend that this is an 
eagerly computed value, without considering exactly when it's computed.

I'm not sure I like this if we do consider it performance sensitive - it 
obfuscates exactly which set of commands we parse, it would be nice if we were 
upfront about this.


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// A CompileCommand that can be applied to another file. Any instance of this
+// object is invalid after std::move() from it.
 struct TransferableCommand {

ilya-biryukov wrote:
> jfb wrote:
> > This comment about `move` isn't really saying anything. Also, it's valid 
> > but unspecified (in the case of STL things). I'd drop it.
> We specifically assert that object cannot be called after `move()` (check the 
> unique_ptr that stores our `once_flag`). It's definitely undefined behavior 
> to call any methods, because they will immediately dereference a null pointer 
> (the aforementioned unique_ptr).
> 
> Happy to drop the comment, though, we do have asserts for that.
I think the idea is "this comment just reiterates standard C++ semantics".

(FWIW I find the asserts hurt readability - it's unusual to try to detect this 
condition, and TraitsComputed is easily mistaken for a boolean)


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D51314#1215381, @sammccall wrote:

> I'm a bit uncomfortable about the places where we need the type, because this 
> is the only thing forcing us to parse before we've picked a command to 
> transfer, and the number of commands we need to parse is data-dependent and 
> hard to reason about.
>
> Let me think about this a little - I suspect slightly more invasive changes 
> (change the concept of type, tweak the heuristics, or do a "lightweight 
> parse" to get the type) might make this cleaner and performance more 
> predictable.


Having studied the code a bit - we use the parsed type to evaluate candidates, 
but we're comparing against the type extracted from the query filename (we have 
nothing else!).
So we should be fine just to compare extensions instead here. So either 
TransferableCommand would have an Extension field or a TypeFromFilename field - 
but we should be careful not to conflate the type inferred from the filename 
(fine for *selecting* a command) with the one parsed from the command (needed 
to *transfer* the command).

Then we have no need to parse for selection, and `getCompileCommands()` only 
needs to parse a single command from the underlying CDB, which should yield 
very predictable/consistent performance.
(It also couples nicely with the idea of only grabbing the full command from 
the underlying CDB lazily - we'll only do that once, too).




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:480-481
 std::vector Result;
-for (auto Command : Inner->getAllCompileCommands()) {
+for (auto Command : Inner->getAllCompileCommands())
   Result.emplace_back(std::move(Command));
 return Result;

as you pointed out offline, we actually only need the filename for indexing, so 
we could ask the underlying DB for the filenames and get their commands on 
demand.

(we need to verify that the values returned don't differ from the filenames 
stored in CompileCommand, e.g. by filename normalization, in a way that matters)


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D51279#1214268, @ilya-biryukov wrote:

> Just noticed I'm not on the reviewers list, sorry for chiming in without 
> invitation. Was really excited about the change :-)


fixed :-)




Comment at: clangd/index/FileIndex.cpp:58
 
-  return Collector.takeSymbols();
+  vlog("index for AST: ");
+  auto Syms = Collector.takeSymbols();

combine into one log statement



Comment at: clangd/index/FileIndex.h:50
 
+  std::vector> allOccurrenceSlabs() 
const;
+

This return type seems inconsistent with the design of this class.
The purpose of the `shared_ptr>` in `allSymbols` is to 
avoid exposing the concrete storage.
The analogue would I guess be `shared_ptr>>`.

The cost of creating that does seem a little gross though.



Comment at: clangd/index/FileIndex.h:50
 
+  std::vector> allOccurrenceSlabs() 
const;
+

sammccall wrote:
> This return type seems inconsistent with the design of this class.
> The purpose of the `shared_ptr>` in `allSymbols` is to 
> avoid exposing the concrete storage.
> The analogue would I guess be `shared_ptr vector>>`.
> 
> The cost of creating that does seem a little gross though.
allOccurrences
(name should reflect the semantics, not the type)



Comment at: clangd/index/FileIndex.h:57
   llvm::StringMap> FileToSlabs;
+  /// \breif Stores the latest occurrence slabs for all active files.
+  llvm::StringMap> FileToOccurrenceSlabs;

nit: no need for brief unless the "first sentence" heuristic fails. (also, it's 
misspelled!)





Comment at: clangd/index/FileIndex.h:95
 
 /// Retrieves namespace and class level symbols in \p AST.
 /// Exposed to assist in unit tests.

update doc



Comment at: clangd/index/MemIndex.cpp:36
   auto Idx = llvm::make_unique();
-  Idx->build(getSymbolsFromSlab(std::move(Slab)));
+  Idx->build(getSymbolsFromSlab(std::move(Slab)), {});
   return std::move(Idx);

we should find a way to avoid having functions implicitly zero out parts of the 
index like this!



Comment at: clangd/index/MemIndex.cpp:86
 const OccurrencesRequest &Req,
 llvm::function_ref Callback) const {
+  for (const auto &Slab : OccurrenceSlabs) {

This assumes there is no duplication across occurrences, but gives no mechanism 
through which deduplication could occur (because we're coupled to the 
underlying storage).

If FileIndex provides the DenseMap> then it 
could just refrain from providing duplicate pointers.



Comment at: clangd/index/MemIndex.h:27
+  build(std::shared_ptr> Symbols,
+std::vector> OccurrenceSlabs = 
{});
 

ilya-biryukov wrote:
> Maybe remove the default parameter? Making the callers specify the 
> occurrences explicitly could make their code more straight-forward to follow.
(type and name: same comment as on FileIndex)



Comment at: clangd/index/Merge.cpp:84
Callback) const override {
-log("findOccurrences is not implemented.");
+// Store all seen files by dynamic index.
+llvm::DenseSet SeenFiles;

I'm tempted to say remove this heuristic for now (as you say, we should solve 
this thoroughly), but the artifacts will be severe, so it's probably correct to 
include it.

But this comment doesn't explain the heuristic, it just echoes the code.
I'd suggest something like:
```// We don't want duplicate occurrences from the static/dynamic indexes,
// and we can't reliably deduplicate because offsets may differ slightly.
// We consider the dynamic index authoritative and report all its occurrences,
// and only report static index occurrences from other files.
// FIXME: this heuristic fails if the dynamic index contains a file, but all
// occurrences were removed (we will report stale ones from static).
// Ultimately we should explicitly check which index has the file instead.```





Comment at: clangd/index/Merge.cpp:85
+// Store all seen files by dynamic index.
+llvm::DenseSet SeenFiles;
+// Emit all dynamic occurrences, and static occurrences that are not

SeenFiles -> DynamicIndexFiles?



Comment at: clangd/index/Merge.cpp:88
+// in seen files.
+// FIXME: If a file has been updated, and there are no occurrences indexed
+// in dynamic index, stale results are still returned (from static index).

hokein wrote:
> ilya-biryukov wrote:
> > Do we want to fix it before checking this in? As discussed offline, this 
> > seems to be a limitation that could affect the design (FileOccurrences vs 
> > SymbolOccurrence in the symbol interface). In case we want to avoid 
> > refactoring the interfaces later.
> Yes, I re-thought it afterwards. We could solve this issue for 
> `findOccurrences` by redefining

[PATCH] D51294: Fix Bug 38713: clang-format mishandles a short block after "default:" in a switch statement

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




Comment at: lib/Format/UnwrappedLineFormatter.cpp:486
   return 0;
+if (Line.First->is(tok::kw_default)) {
+  const FormatToken *Tok = Line.First->getNextNonComment();

Just to check my understanding...
we want to treat `default` the same as `case`, but the heuristics are different:
 - `case` should only appear in a switch (but might be followed by a complex 
expression)
 - `default` has lots of meanings (but we can disambiguate: check if it's 
followed by a colon)

You could consider `// default: in switch statement` above this line.



Comment at: unittests/Format/FormatTest.cpp:1012
+   "{\n"
+   "case 0: {\n"
+   "  return false;\n"

the intent of this test might be clearer if the cases were formatted as `case 
0: { return false; }` on one line


Repository:
  rC Clang

https://reviews.llvm.org/D51294



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

[PATCH] D51310: [clangd] Implement iterator cost

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

In https://reviews.llvm.org/D51310#1214548, @kbobyrev wrote:

> It's probably better to roll out proximity path boosting & actual two-stage 
> filtering before rolling this out.


Up to you (I don't understand the interaction), but this looks good to go 
as-is. I'd expect a 10% speedup to be pretty robust to the sorts of changes 
you're talking about.




Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:90
 sync();
+std::sort(begin(Children), end(Children),
+  [](const std::unique_ptr &LHS,

this is a performance heuristic and deserves a brief comment (about *why* we're 
best to have the shortest first). Not too many details, handwavy is fine :-)



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:93
+ const std::unique_ptr &RHS) {
+return LHS->cost() < RHS->cost();
+  });

(I'd actually suggest declaring operator< in iterator.h and making it compare 
by cost, it seems natural/important enough, and saves duplicating this lambda)



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:125
+  size_t cost() override {
+return std::accumulate(
+begin(Children), end(Children), Children.front()->cost(),

The use of `std::accumulate` here seems less clear than the direct:
```size_t Smallest = ...;
for (const auto& Child : Children)
  Smallest = std::min(Smallest, Child->cost());
return Smallest;
```
For better or worse, functional styles don't have the most natural syntax in 
C++, and (IMO) shouldn't be used just because they fit.

(This is consistent with other places, but I also think that the same applies 
there)



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:125
+  size_t cost() override {
+return std::accumulate(
+begin(Children), end(Children), Children.front()->cost(),

sammccall wrote:
> The use of `std::accumulate` here seems less clear than the direct:
> ```size_t Smallest = ...;
> for (const auto& Child : Children)
>   Smallest = std::min(Smallest, Child->cost());
> return Smallest;
> ```
> For better or worse, functional styles don't have the most natural syntax in 
> C++, and (IMO) shouldn't be used just because they fit.
> 
> (This is consistent with other places, but I also think that the same applies 
> there)
actually, we've sorted by cost, so don't we just want Children.front().cost() 
here?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:258
 
+  size_t cost() override {
+return std::accumulate(

as above, we've sorted, so just Children.back()->cost()?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:377
 
+  size_t cost() override { return Limit; }
+

min() this with Child->cost()?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:99
+  /// Returns an estimate of advance() calls before the iterator is exhausted.
+  virtual size_t cost() = 0;
 

const?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:99
+  /// Returns an estimate of advance() calls before the iterator is exhausted.
+  virtual size_t cost() = 0;
 

sammccall wrote:
> const?
I know this terminology comes from elsewhere, but I struggle with "cost" as a 
metaphor because I expect it to aggregate its children as a sum (at least in 
cases like "or" when all children will be advanced until end).

Maybe `estimateSize()`?


https://reviews.llvm.org/D51310



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Awesome  :-)




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:220
 
-// CommandIndex does the real work: given a filename, it produces the best
-// matching TransferableCommand by matching filenames. Basic strategy:
+// FileProximityIndex chooses the proxy file that has a compile command in a
+// compilation database when given a file that is not in the database. Basic

This summary is a bit unclear to me. (too many clauses, maybe too abstract).
And the high level heuristics are hidden a bit below the implementation ideas.

Maybe

```
Given a filename, FileProximityIndex picks the best matching file from the 
underlying DB. This is the proxy file whose CompileCommand will be reused.

The heuristics incorporate file name, extension, and directory structure.

Strategy:
...```



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:220
 
-// CommandIndex does the real work: given a filename, it produces the best
-// matching TransferableCommand by matching filenames. Basic strategy:
+// FileProximityIndex chooses the proxy file that has a compile command in a
+// compilation database when given a file that is not in the database. Basic

sammccall wrote:
> This summary is a bit unclear to me. (too many clauses, maybe too abstract).
> And the high level heuristics are hidden a bit below the implementation ideas.
> 
> Maybe
> 
> ```
> Given a filename, FileProximityIndex picks the best matching file from the 
> underlying DB. This is the proxy file whose CompileCommand will be reused.
> 
> The heuristics incorporate file name, extension, and directory structure.
> 
> Strategy:
> ...```
nit: I'd prefer `FileIndex` or `FilenameIndex` here - "proximity" emphasizes 
directory structure over stem/extension, which are pretty important!



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:232
-  : Commands(std::move(AllCommands)), Strings(Arena) {
-// Sort commands by filename for determinism (index is a tiebreaker later).
-llvm::sort(

restore this comment?



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:338
   S.Preferred = PreferredLanguage == types::TY_INVALID ||
-PreferredLanguage == Commands[S.Index].Type;
+PreferredLanguage == guessType(Paths[S.Index].first);
   S.Points = Candidate.second;

hmm, I would have thought we'd store the values of guessType() when building 
the index. I guess it doesn't matter, it just seems surprising to see this call 
here.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:338
   S.Preferred = PreferredLanguage == types::TY_INVALID ||
-PreferredLanguage == Commands[S.Index].Type;
+PreferredLanguage == guessType(Paths[S.Index].first);
   S.Points = Candidate.second;

sammccall wrote:
> hmm, I would have thought we'd store the values of guessType() when building 
> the index. I guess it doesn't matter, it just seems surprising to see this 
> call here.
you're calling foldType(guessType(...)) on the query, do you need to fold here 
too?



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:417
 bool TypeCertain;
 auto Lang = guessType(Filename, &TypeCertain);
 if (!TypeCertain)

this guessType/foldType call should be folded into Index.chooseProxy now I 
think - Index explicitly knows that the language it deals with must be derived 
from the filename.


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51349: [clangd] Use buffered llvm::errs() in the clangd binary.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/tool/ClangdMain.cpp:263
+  // Use buffered stream to stderr. Unbuffered stream can cause significant
+  // (non-deterministic) latency for the logger.
+  llvm::errs().SetBuffered();

(we still flush each log message).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51349



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

[PATCH] D51291: [clangd] *Prototype* Support multiple #include headers in one symbol.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!

We could reduce the scope of this patch somewhat by ignoring file proximity and 
just switching to return the most popular header. This would be a solid 
improvement over current behavior, and provide the infrastructure for the 
file-proximity approach.




Comment at: clangd/CodeComplete.cpp:1396
+  if (IndexResult && !IndexResult->IncludeHeaders.empty()) {
+for (const auto &P : IndexResult->IncludeHeaders)
+  AddWithInclude(P);

I remain unconvinced that providing multiple completion candidates is the right 
thing to do here:
 - if the index hasn't seen a definition, then we're going to show one copy of 
the completion for each header that has a declaration
 - the user isn't going to have a useful way to distinguish between them. Note 
that e.g. we're going to show the #include path in the detail, but the 
documentation is going to be identical for each.
 - we lose the invariant that each completion item (pre-bundling) maps to a 
different symbol
 - C++ embedders don't have the option of displaying the multiple options once 
the completion is selected

The alternative approach of sorting the includes by proximity * log(refs) or 
so, and then using the top one for scoring, seems less of a drastic change to 
the current behavior. (Visible effect: more accurate includes inserted).



Comment at: clangd/Quality.cpp:190
 
+static const float kIncludeHeaderScoreIncreaseUnit = 0.0001;
+

This looks a bit sketchy. Particularly the += boost where everything else is *=.
What's this trying to do?



Comment at: clangd/index/Index.h:220
+llvm::StringRef IncludeHeader = "";
+/// The number of translation units that reference this symbol via this
+/// header. This number is only meaningful if aggregated in an index.

via this header -> and include this header?

(otherwise, what does "via" mean?)



Comment at: clangd/index/Merge.cpp:105
+  // merge include headers from L and R, as they can both export the symbol.
+  bool MergeIncludes = !L.Definition.FileURI.empty() &&
+   !R.Definition.FileURI.empty() &&

This describes the logic, and the logic always produces the right result, but 
it's not clear *why*. Maybe add something like:

```This is associative because it preserves the invariant:
 - if we haven't seen a definition, Includes covers all declarations
 - if we have seen a definition, Includes covers declarations visible from any 
definition```

in fact it seems hard to reason about this field in Symbol without 
understanding this, so maybe this invariant belongs on the IncludeHeaders field 
itself.



Comment at: clangd/index/Merge.cpp:105
+  // merge include headers from L and R, as they can both export the symbol.
+  bool MergeIncludes = !L.Definition.FileURI.empty() &&
+   !R.Definition.FileURI.empty() &&

sammccall wrote:
> This describes the logic, and the logic always produces the right result, but 
> it's not clear *why*. Maybe add something like:
> 
> ```This is associative because it preserves the invariant:
>  - if we haven't seen a definition, Includes covers all declarations
>  - if we have seen a definition, Includes covers declarations visible from 
> any definition```
> 
> in fact it seems hard to reason about this field in Symbol without 
> understanding this, so maybe this invariant belongs on the IncludeHeaders 
> field itself.
Thinking more about it - what's the intent here?
I'm not sure sorting by (seen by def, #refs) produces better ranking than just 
#refs.

But there are other possible reasons for dropping includes not seen by any def:
 - remove spam from the completion list (only a problem if we clone the 
completion items)
 - reduce size for items that are often redeclared (I can imagine this being a 
problem, not obvious)

Curious what your thinking is.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51291



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:229
+//
+// Apart from path proximity signals, also takes file extensions into account
+// when scoring the candidates.

(this comment is covered in the summary now)


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hi, sorry about overlooking cl mode flags when adding this, I was blissfully 
unaware that `compile_commands.json` could use that syntax :-)

Out of curiosity, are you using this with clangd or some other tool? I'm sure 
there are places where clangd injects unixy flags...
Will take a look and try to understand.

Be aware of https://reviews.llvm.org/D51314 which is fixing some nasty 
performance pitfalls of InterpolatingCompilationDatabase (it should be logic 
neutral, but moves around the parsing code).

Could you reupload the patch with context?


https://reviews.llvm.org/D51321



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

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:127
   // Language detected from -x or the filename.
-  types::ID Type = types::TY_INVALID;
+  // When set, cannot be TY_INVALID.
+  llvm::Optional Type;

(or just "never TY_INVALID" which would fit on prev line :-)



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:175
   Type = toType(LangStandard::getLangStandardForKind(Std).getLanguage());
-Type = foldType(Type);
+if (Type)
+  Type = foldType(*Type);

it's always set here, drop the condition.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:408
   BumpPtrAllocator Arena;
-  StringSaver Strings;
+  llvm::StringSaver Strings;
   // Indexes of candidates by certain substrings.

nit: no llvm:: :-)


Repository:
  rC Clang

https://reviews.llvm.org/D51314



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

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for fixing this!
Mostly just picky style comments.
In particular, I know that some of the other maintainers here are just as 
ignorant of the cl driver as I am, and I want to make sure that it's still 
possible to follow the logic and debug unrelated problems without needing to 
know too much about it.

If some of the style bits is too annoying or unclear, happy to do some of it 
myself as a followup, let me know!




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156-169
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as 
the
+// arg list contains pointers to the OldArgs strings.
+llvm::opt::InputArgList ArgList;
+{
+  // Unfortunately InputArgList requires an array of c-strings whereas we
+  // have an array of std::string; we'll need an intermediate vector.

hamzasood wrote:
> rnk wrote:
> > I think the old for loop was nicer. I don't think this code needs to 
> > change, you should be able to use ParseArgs with the extra flag args.
> The problem with the old loop is that we lose aliasing information (e.g. 
> `/W3` becomes `-Wall`). While `ParseOneArg` also performs alias conversion, 
> it gives us indices for each individual argument. The last line of the new 
> loop copies arguments by using that index information to read from `OldArgs`, 
> which gives us the original spelling.
Makes sense, please add a comment summarizing. ("We don't use ParseArgs, as we 
must pass through the exact spelling of uninteresting args. Re-rendering is 
lossy for clang-cl options, e.g. /W3 -> -Wall")



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector &CmdLine,
+  const llvm::opt::OptTable &OptTable) {

nit: a two-value enum would be clearer and allow for terser variable names 
(`Mode`)



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156
+
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as 
the

would you mind reverting this change and just wrapping the old Argv in an 
InputArgList?
I'm not sure the lifetime comments and std::transform aid readability here.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:175
+Cmd.CommandLine.emplace_back(OldArgs.front());
+for (unsigned Start = 1, End = Start; End < OldArgs.size(); Start = End) {
+  std::unique_ptr Arg;

these names are somewhat confusing - "End" neither marks the end of the loop 
nor the end of the current item (as it's initialized to Start).
What about:
```
for (unsigned Pos = 1; Pos < OldArgs.size();) {
  ...
  unsigned OldPos = Pos;
  Arg = ParseOneArg(..., Pos);
  ...
  NewArgs.insert(NewArgs.end(), &OldArgs[OldPos], &OldArgs[Pos]);
}
```



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:178
+  {
+unsigned Included, Excluded;
+if (IsCLMode) {

This seems a bit verbose.

First, do things actually break if we just use the default 0/0 masks? We're not 
trying to interpret all the flags, just a few and pass the rest through.

Failing that, it seems clearer to just use a ternary to initialize 
Included/Excluded, or inline them completely.
(Please also drop the extra scope here).



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:186
+}
+Arg = std::unique_ptr(
+  OptTable->ParseOneArg(ArgList, End, Included, Excluded));

Arg.reset(OptTable->ParseOneArg...)



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:194
   // Strip input and output files.
-  if (option.matches(clang::driver::options::OPT_INPUT) ||
-  option.matches(clang::driver::options::OPT_o)) {
+  if (isUntypedInputOrOutput(*Arg))
 continue;

can we inline this and just `||` all the options together here?



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:197
+
+  // Strip type specifiers, but record the overridden language.
+  if (const auto GivenType = isTypeSpecArg(*Arg)) {

please keep the mentions of -x etc even if not comprehensive - it's hard to 
precisely+tersely describe these flags in prose.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:203
+
+  // Strip std, but record the value.
+  if (const auto GivenStd = isStdArg(*Arg)) {

ditto --std



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:205
+  if (const auto GivenStd = isStdArg(*Arg)) {
+if (*GivenStd != LangStandard::lang_unspecified)
+   

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.



Comment at: clangd/ClangdServer.cpp:122
+  //  - symbols declared both in the main file and the preamble
+  // (Note that symbols *only* in the main file are not indexed).
   FileIndex MainFileIdx;

ilya-biryukov wrote:
> I thought it was not true, but now I notice we actually don't index those 
> symbols.
> I think we should index them for workspaceSymbols, but not for completion. 
> Any pointers to the code that filters them out?
https://reviews.llvm.org/source/clang-tools-extra/browse/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp;340828$272

isExpansionInMainFile()

(having this buried so deep hurts readability and probably performance).

But I think this would be the behavior we want for code complete, to keep the 
indexes tiny and incremental updates fast?
WorkspaceSymbols is indeed a problem here :-( Tradeoffs...



Comment at: clangd/ClangdServer.cpp:152
   WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
 Opts.UpdateDebounce, Opts.RetentionPolicy) {

ilya-biryukov wrote:
> Any reason to prefer `nullptr` over the no-op callbacks?
> Might be a personal preference, my reasoning is: having an extra check for 
> null before on each of the call sites both adds unnecessary boilerplate and 
> adds an extra branch before the virtual call (which is, technically, another 
> form of a branch).
> 
> Not opposed to doing it either way, though.
Basically I prefer the old behavior here (when it was std::function).
Having to keep the callbacks object alive is a pain, and the shared instance of 
the no-op implementation for this purpose seems a little silly.

We don't have the std::function copyability stuff which is a mixed bag but nice 
for cases like this. But passing the reference from TUScheduler to its 
ASTWorkers is internal enough that it doesn't bother me, WDYT?

> extra check for null before on each of the call sites 
Note that the check is actually in the constructor, supporting `nullptr` is 
just for brevity (particularly in tests).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221



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

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/TUScheduler.cpp:632
  bool StorePreamblesInMemory,
- ParsingCallbacks &Callbacks,
+ std::unique_ptr Callbacks,
  std::chrono::steady_clock::duration UpdateDebounce,

ilya-biryukov wrote:
> Why not `ParsingCallbacks*` or `ParsingCallbacks&`? This restricts the 
> lifetime of the passed values.
> Our particular use-case LG this way, but it seems there is no reason why 
> `TUScheduler` should own the callbacks.
As noted above, this is just like std::function.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221



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

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163013.
sammccall marked an inline comment as done.
sammccall added a comment.

Address review comments, add missing dynamicIndex() implementation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx,
-  std::shared_ptr PP) {
+  [&](ASTContext &Ctx, std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, &Ctx, std::move(PP));
+Index.update(FooCpp, &Ctx, std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
==

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163023.
sammccall added a comment.

(forgot some changes)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx,
-  std::shared_ptr PP) {
+  [&](ASTContext &Ctx, std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, &Ctx, std::move(PP));
+Index.update(FooCpp, &Ctx, std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUSc

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, 
MaskRay, ioeric, ilya-biryukov.

This is now handled by a wrapper class SwapIndex, so MemIndex/DexIndex can be
immutable and focus on their job.

Old and busted:
 I have a MemIndex, which holds a shared_ptr>, which keeps the
 symbol slab alive. I update by calling build(shared_ptr>).

New hotness: I have a SwapIndex, which holds a shared_ptr, which
 holds a MemIndex and also keeps any data backing it alive.
 I update by building a new MemIndex and calling SwapIndex::reset().

This resulted in a bunch of interface churn (some places previously using
unique_ptr should now be shared_ptr, and some using MemIndex() + MemIndex::build
should now call the static MemIndex::build factory instead).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422

Files:
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/MemIndex.h
  clangd/index/dex/DexIndex.cpp
  clangd/index/dex/DexIndex.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexIndexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestIndex.cpp
  unittests/clangd/TestIndex.h
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -51,7 +51,7 @@
 
   ParsedAST build() const;
   SymbolSlab headerSymbols() const;
-  std::unique_ptr index() const;
+  std::shared_ptr index() const;
 };
 
 // Look up an index symbol by qualified name, which must be unique.
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -48,7 +48,7 @@
   return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
 }
 
-std::unique_ptr TestTU::index() const {
+std::shared_ptr TestTU::index() const {
   return MemIndex::build(headerSymbols());
 }
 
Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
 // Creates Symbol instance and sets SymbolID to given QualifiedName.
 Symbol symbol(llvm::StringRef QName);
 
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
-  SymbolSlab Slab;
-  std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
 
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
 
 // Returns fully-qualified name out of given symbol.
 std::string getQualifiedName(const Symbol &Sym);
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
   return Sym;
 }
 
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
   SymbolSlab::Builder Slab;
   for (llvm::StringRef QName : QualifiedNames)
 Slab.insert(symbol(QName));
-
-  auto Storage = std::make_shared();
-  Storage->Slab = std::move(Slab).build();
-  for (const auto &Sym : Storage->Slab)
-Storage->Pointers.push_back(&Sym);
-  if (WeakSymbols)
-*WeakSymbols = Storage;
-  auto *Pointers = &Storage->Pointers;
-  return {std::move(Storage), Pointers};
+  return std::move(Slab).build();
 }
 
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
   std::vector Names;
   for (int i = Begin; i <= End; i++)
 Names.push_back(std::to_string(i));
-  return generateSymbols(Names, WeakSymbols);
+  return generateSymbols(Names);
 }
 
 std::string getQualifiedName(const Symbol &Sym) {
Index: unittests/clangd/Inde

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, javed.absar.

After code completion inserts a header, running signature help using the old
preamble will usually fail. So we add support for consistent preamble reads.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51438

Files:
  clangd/ClangdServer.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -63,7 +63,7 @@
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Missing,
+  S.runWithPreamble("", Missing, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   ASSERT_FALSE(bool(Preamble));
   ignoreError(Preamble.takeError());
@@ -75,20 +75,22 @@
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 EXPECT_TRUE(bool(AST));
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-EXPECT_TRUE(bool(Preamble));
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  EXPECT_TRUE(bool(Preamble));
+});
   S.remove(Added);
 
   // Assert that all operations fail after removing the file.
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-ASSERT_FALSE(bool(Preamble));
-ignoreError(Preamble.takeError());
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  ASSERT_FALSE(bool(Preamble));
+  ignoreError(Preamble.takeError());
+});
   // remove() shouldn't crash on missing files.
   S.remove(Added);
 }
@@ -229,7 +231,7 @@
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.runWithPreamble(
-  "CheckPreamble", File,
+  "CheckPreamble", File, TUScheduler::Stale,
   [File, Inputs, Nonce, &Mut, &TotalPreambleReads](
   llvm::Expected Preamble) {
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
@@ -324,7 +326,7 @@
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
[](std::vector) {});
-  S.runWithPreamble("getNonEmptyPreamble", Foo,
+  S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   // We expect to get a non-empty preamble.
   EXPECT_GT(cantFail(std::move(Preamble))
@@ -340,7 +342,7 @@
[](std::vector) {});
   // Wait for the preamble is being built.
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  S.runWithPreamble("getEmptyPreamble", Foo,
+  S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   // We expect to get an empty preamble.
   EXPECT_EQ(cantFail(std::move(Preamble))
@@ -372,7 +374,7 @@
[](std::vector) {});
   for (int I = 0; I < ReadsToSchedule; ++I) {
 S.runWithPreamble(
-"test", Foo,
+"test", Foo, TUScheduler::Stale,
 [I, &PreamblesMut, &Preambles](llvm::Expected IP) {
   std::lock_guard Lock(PreamblesMut);
   Preambles[I] = cantFail(std::move(IP)).Preamble;
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -119,19 +119,28 @@
   void runWithAST(llvm::StringRef Name, PathRef File,
   Callback Action);
 
-  /// Schedule an async read of the Preamble.
-  /// The preamble may be stale, generated from an older version of the file.
-  /// Reading from locations in the preamble may cause the files to be re-read.
-  /// This gives callers two options:
-  /// - validate that the preamble is still valid, and only use it in this case
-  /// - accept that preamble contents may be outdated, and try to avoid reading
-  ///   source code from headers.
+  /// Controls whether preamble reads 
+  enum PreambleConsistency {
+/// The preamble is generated from the current version of the file.
+/// If the content was recently updated, we will wait until we have a
+/// preamble that reflects that update.
+/// This is the slowest option, and may be delayed by other tasks.
+Consistent,
+/// The preamble may be generated from an older version of the file.
+/// Reading from locations in the preamble may cause files to 

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(This needs new TUScheduler tests for `Consistent` mode, but would like some 
feedback on the implementation first)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51438



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

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163121.
sammccall added a comment.

Finish a sentence


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51438

Files:
  clangd/ClangdServer.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -63,7 +63,7 @@
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Missing,
+  S.runWithPreamble("", Missing, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   ASSERT_FALSE(bool(Preamble));
   ignoreError(Preamble.takeError());
@@ -75,20 +75,22 @@
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 EXPECT_TRUE(bool(AST));
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-EXPECT_TRUE(bool(Preamble));
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  EXPECT_TRUE(bool(Preamble));
+});
   S.remove(Added);
 
   // Assert that all operations fail after removing the file.
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-ASSERT_FALSE(bool(Preamble));
-ignoreError(Preamble.takeError());
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  ASSERT_FALSE(bool(Preamble));
+  ignoreError(Preamble.takeError());
+});
   // remove() shouldn't crash on missing files.
   S.remove(Added);
 }
@@ -229,7 +231,7 @@
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.runWithPreamble(
-  "CheckPreamble", File,
+  "CheckPreamble", File, TUScheduler::Stale,
   [File, Inputs, Nonce, &Mut, &TotalPreambleReads](
   llvm::Expected Preamble) {
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
@@ -324,7 +326,7 @@
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
[](std::vector) {});
-  S.runWithPreamble("getNonEmptyPreamble", Foo,
+  S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   // We expect to get a non-empty preamble.
   EXPECT_GT(cantFail(std::move(Preamble))
@@ -340,7 +342,7 @@
[](std::vector) {});
   // Wait for the preamble is being built.
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  S.runWithPreamble("getEmptyPreamble", Foo,
+  S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   // We expect to get an empty preamble.
   EXPECT_EQ(cantFail(std::move(Preamble))
@@ -372,7 +374,7 @@
[](std::vector) {});
   for (int I = 0; I < ReadsToSchedule; ++I) {
 S.runWithPreamble(
-"test", Foo,
+"test", Foo, TUScheduler::Stale,
 [I, &PreamblesMut, &Preambles](llvm::Expected IP) {
   std::lock_guard Lock(PreamblesMut);
   Preambles[I] = cantFail(std::move(IP)).Preamble;
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -119,19 +119,28 @@
   void runWithAST(llvm::StringRef Name, PathRef File,
   Callback Action);
 
-  /// Schedule an async read of the Preamble.
-  /// The preamble may be stale, generated from an older version of the file.
-  /// Reading from locations in the preamble may cause the files to be re-read.
-  /// This gives callers two options:
-  /// - validate that the preamble is still valid, and only use it in this case
-  /// - accept that preamble contents may be outdated, and try to avoid reading
-  ///   source code from headers.
+  /// Controls whether preamble reads wait for the preamble to be up-to-date.
+  enum PreambleConsistency {
+/// The preamble is generated from the current version of the file.
+/// If the content was recently updated, we will wait until we have a
+/// preamble that reflects that update.
+/// This is the slowest option, and may be delayed by other tasks.
+Consistent,
+/// The preamble may be generated from an older version of the file.
+/// Reading from locations in the preamble may cause files to be re-read.
+/// This gives callers two options:
+/// - validate that the preamble is still valid, and only use it if so
+/// - accept that the preamble contents may be outdated, and tr

[PATCH] D51437: [clangd] Report position of opening paren in singature help

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/Protocol.h:832
+
+  /// Position of the opening paren of the argument list.
+  /// This is a clangd-specific extension, it is only available via C++ API and

As noted offline, I misread this as being *inside* the paren - could maybe be 
more explicit?
```Position of the start of the argument list, including opening paren. e.g.
foo("first arg", 
   ^argListStart ^cursor
```



Comment at: unittests/clangd/CodeCompleteTests.cpp:914
 
+TEST(SignatureHelpTest, OpeningParen) {
+  Annotations Code(R"cpp(

Hmm, I think this test would be easier to follow if tests 1-5 were written 
separately - it's hard to spot all the locations and how the code interacts.

As a bonus, no need to mess around with explicit positions and the failure 
message can just echo the test:

```
for (const char* Test : {
  R"cpp(
int foo(int a, b, c);
int main() { foo(foo$p^(foo(10, 10, 10), ^); }
  )cpp",
  ...
}) {
  EXPECT_EQ(signatures(Test).argListStart, Annotations(Test).point("p")) << 
Test;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51437



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

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

- It would be useful for the C++ API (CodeCompletion struct) to provide the 
includes/edits in ranked order, if possible. Frontends could experiment with 
showing all the options.
- Still not sure that adding more complicated behavior to Code Complete (vs 
just improving ranking) is the right thing for now (bigger comment below)




Comment at: clangd/CodeComplete.cpp:303
+
+// Pick the most popular include, only if it has way more references than
+// the rest of includes; otherwise, we just give up and don't insert

I'm not sure about this:
 - we're adding new user-visible behavior which is potentially confusing (not 
inserting even though insertion is turned on and we're offering a completion 
that requires insertion)
 - it's not clear that first > 10 * second is the right heuristic or tuning
 - this probably isn't the right place for this heuristic - e.g. it causes us 
not to show the location of the symbol in the detail field. (IIRC, the fact 
that this is only shown when header insertion is active is a limitation we 
wanted to fix, not furither entrench).

Can we start with the simplest behavior, closest to what we do today: just 
choose the most popular include?

Probably the most forward-looking way to do this is to pick the winner when we 
construct the CompletionCandidate and stash it in a field there. (The scoring 
function is going to become more complex as we add proximity, and this function 
gets called twice. Also this function won't have access to the info needed for 
proximity).



Comment at: clangd/CodeComplete.cpp:316
+  });
+if (Includes.begin()->References <
+10 * std::next(Includes.begin())->References)

nit: Includes[0] and Includes[1]?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51291



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

[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:45
+  void build(std::shared_ptr> Symbols,
+ llvm::ArrayRef URISchemes);
 

ioeric wrote:
> URI schemes are property of `Symbols`. We shouldn't need to pass specify the 
> schemes again. Dex can collect all possible URI schemes when building the 
> index.
> 
> I think we could generate proximity paths for index symbols without knowing 
> about schemes (when building the index). For example, we could `canonicalize` 
> the URI (as in `FileDistance.cpp`), for example, by converting 
> `test:///x/y/z` to `/test:/x/y/z`.  For a query, we would first convert query 
> proximity paths to URIs (of all possible schemes), perform the same 
> canonicalization on URIs, and then use the canonicalized paths to compute 
> proximity.
We had an offline discussion about URIs which might be interesting.

Fetching posting lists for all URI schemes at query time seems wasteful, 
@ilya-biryukov pointed out that in practice each file is going to exist in some 
preferred URI space, and we should only compare that one.
The only obstacle to doing that is that the preference order for URI schemes is 
not global, each place we need it it's accepted as configuration.

Maybe we should change that: as part of registering the URI schemes, we define 
a preferred order. And instead of the current operation `makeURI(File, scheme)` 
we should just offer APIs like `makePreferredURI(File)` and `makeFileURI(File)`.


https://reviews.llvm.org/D51481



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

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163322.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Herald added a subscriber: jfb.

Add test, address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51438

Files:
  clangd/ClangdServer.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -22,6 +22,7 @@
 using ::testing::_;
 using ::testing::AnyOf;
 using ::testing::Each;
+using ::testing::ElementsAre;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -63,7 +64,7 @@
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Missing,
+  S.runWithPreamble("", Missing, TUScheduler::Stale,
 [&](llvm::Expected Preamble) {
   ASSERT_FALSE(bool(Preamble));
   ignoreError(Preamble.takeError());
@@ -75,20 +76,22 @@
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 EXPECT_TRUE(bool(AST));
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-EXPECT_TRUE(bool(Preamble));
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  EXPECT_TRUE(bool(Preamble));
+});
   S.remove(Added);
 
   // Assert that all operations fail after removing the file.
   S.runWithAST("", Added, [&](llvm::Expected AST) {
 ASSERT_FALSE(bool(AST));
 ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected Preamble) {
-ASSERT_FALSE(bool(Preamble));
-ignoreError(Preamble.takeError());
-  });
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+[&](llvm::Expected Preamble) {
+  ASSERT_FALSE(bool(Preamble));
+  ignoreError(Preamble.takeError());
+});
   // remove() shouldn't crash on missing files.
   S.remove(Added);
 }
@@ -148,6 +151,64 @@
   EXPECT_EQ(2, CallbackCount);
 }
 
+static std::vector includes(const PreambleData *Preamble) {
+  std::vector Result;
+  if (Preamble)
+for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
+  Result.push_back(Inclusion.Written);
+  return Result;
+}
+
+TEST_F(TUSchedulerTests, PreambleConsistency) {
+  std::atomic CallbackCount(0);
+  {
+Notification InconsistentReadDone; // Must live longest.
+TUScheduler S(
+getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+noopParsingCallbacks(),
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
+auto Path = testPath("foo.cpp");
+// Schedule two updates (A, B) and two preamble reads (stale, consistent).
+// The stale read should see A, and the consistent read should see B.
+// (We recognize the preambles by their included files).
+S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes,
+ [&](std::vector Diags) {
+   // This callback runs in between the two preamble updates.
+
+   // This blocks update B, preventing it from winning the race
+   // against the stale read.
+   // If the first read was instead consistent, this would deadlock.
+   InconsistentReadDone.wait();
+   // This delays update B, preventing it from winning a race
+   // against the consistent read. The consistent read sees B
+   // only because it waits for it.
+   // If the second read was stale, it would usually see A.
+   std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ });
+S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes,
+ [&](std::vector Diags) {});
+
+S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
+  [&](llvm::Expected Pre) {
+ASSERT_TRUE(bool(Pre));
+assert(bool(Pre));
+EXPECT_THAT(includes(Pre->Preamble),
+ElementsAre(""));
+InconsistentReadDone.notify();
+++CallbackCount;
+  });
+S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
+  [&](llvm::Expected Pre) {
+ASSERT_TRUE(bool(Pre));
+EXPECT_THAT(includes(Pre->Preamble),
+ElementsAre(""));
+++CallbackCount;
+  });
+  }
+  EXPECT_EQ(2, CallbackCount);
+}
+
 TEST_F(TUSchedulerTests, ManyUpdates) {
   const int FilesCount = 3;
   const int UpdatesPerFile = 10;
@@ -229,7 +290,7 

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I don't have a strong opinion on async vs sync - startup time is important and 
we shouldn't block simple AST-based functionality on the index, but this 
introduces some slightly confusing UX for that speed.

However I think this should be based on https://reviews.llvm.org/D51422 which 
extracts most of what you need out of MemIndex into the new `SwapIndex`.
So static index would just be initialized as an empty SwapIndex, then spawn a 
thread that loads the YAML and calls SwapIndex::reset.
This will get avoid adding nontrivial threading stuff to the main file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51475



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

[PATCH] D51475: [clangd] Load YAML static index asynchronously.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(Sorry for catching this earlier, and that the patch isn't landed yet - feel 
free to pick up the review, else @kbobyrev will take a first pass tomorrow I 
think)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51475



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

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341076: [clangd] Run SignatureHelp using an up-to-date 
preamble, waiting if needed. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51438

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.h
  clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Index: clang-tools-extra/trunk/clangd/TUScheduler.h
===
--- clang-tools-extra/trunk/clangd/TUScheduler.h
+++ clang-tools-extra/trunk/clangd/TUScheduler.h
@@ -119,19 +119,28 @@
   void runWithAST(llvm::StringRef Name, PathRef File,
   Callback Action);
 
-  /// Schedule an async read of the Preamble.
-  /// The preamble may be stale, generated from an older version of the file.
-  /// Reading from locations in the preamble may cause the files to be re-read.
-  /// This gives callers two options:
-  /// - validate that the preamble is still valid, and only use it in this case
-  /// - accept that preamble contents may be outdated, and try to avoid reading
-  ///   source code from headers.
+  /// Controls whether preamble reads wait for the preamble to be up-to-date.
+  enum PreambleConsistency {
+/// The preamble is generated from the current version of the file.
+/// If the content was recently updated, we will wait until we have a
+/// preamble that reflects that update.
+/// This is the slowest option, and may be delayed by other tasks.
+Consistent,
+/// The preamble may be generated from an older version of the file.
+/// Reading from locations in the preamble may cause files to be re-read.
+/// This gives callers two options:
+/// - validate that the preamble is still valid, and only use it if so
+/// - accept that the preamble contents may be outdated, and try to avoid
+///   reading source code from headers.
+/// This is the fastest option, usually a preamble is available immediately.
+Stale,
+  };
+  /// Schedule an async read of the preamble.
   /// If there's no preamble yet (because the file was just opened), we'll wait
-  /// for it to build. The preamble may still be null if it fails to build or is
-  /// empty.
-  /// If an error occurs during processing, it is forwarded to the \p Action
-  /// callback.
+  /// for it to build. The result may be null if it fails to build or is empty.
+  /// If an error occurs, it is forwarded to the \p Action callback.
   void runWithPreamble(llvm::StringRef Name, PathRef File,
+   PreambleConsistency Consistency,
Callback Action);
 
   /// Wait until there are no scheduled or running tasks.
Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -183,6 +183,10 @@
   bool blockUntilIdle(Deadline Timeout) const;
 
   std::shared_ptr getPossiblyStalePreamble() const;
+  /// Obtain a preamble reflecting all updates so far. Threadsafe.
+  /// It may be delivered immediately, or later on the worker thread.
+  void getCurrentPreamble(
+  llvm::unique_function)>);
   /// Wait for the first build of preamble to finish. Preamble itself can be
   /// accessed via getPossibleStalePreamble(). Note that this function will
   /// return after an unsuccessful build of the preamble too, i.e. result of
@@ -464,6 +468,34 @@
   return LastBuiltPreamble;
 }
 
+void ASTWorker::getCurrentPreamble(
+llvm::unique_function)> Callback) {
+  // We could just call startTask() to throw the read on the queue, knowing
+  // it will run after any updates. But we know this task is cheap, so to
+  // improve latency we cheat: insert it on the queue after the last update.
+  std::unique_lock Lock(Mutex);
+  auto LastUpdate =
+  std::find_if(Requests.rbegin(), Requests.rend(),
+   [](const Request &R) { return R.UpdateType.hasValue(); });
+  // If there were no writes in the queue, the preamble is ready now.
+  if (LastUpdate == Requests.rend()) {
+Lock.unlock();
+return Callback(getPossiblyStalePreamble());
+  }
+  assert(!RunSync && "Running synchronously, but queue is non-empty!");
+  Requests.insert(LastUpdate.base(),
+  Request{Bind(
+  [this](decltype(Callback) Callback) {
+Callback(getPossiblyStalePreamble());
+  },
+  std::move(Callback)),
+  "GetPreamble", steady_clock::now(),
+  Context::current().clone(),
+  /*UpdateType=*/llvm::None});
+  Lock.unlock();
+  RequestsCV.notify_all();
+}

[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/TUScheduler.cpp:474
+llvm::unique_function)> Callback) 
{
+  if (RunSync)
+return Callback(getPossiblyStalePreamble());

ilya-biryukov wrote:
> It seems we could remove the special-casing of `RunSync` and still get 
> correct results (the Requests queue is always empty).
> But feel free to keep it for clarity.
Right, of course :-)
Replaced this with an assert before we write to the queue.



Comment at: clangd/TUScheduler.h:123
+  /// Controls whether preamble reads wait for the preamble to be up-to-date.
+  enum PreambleConsistency {
+/// The preamble is generated from the current version of the file.

ilya-biryukov wrote:
> Maybe use a strongly-typed enum outside the class?
> `TUScheduler::Stale` will turn into `PreambleConsistency::Stale` at the 
> call-site. The main upside is that it does not pollute completions inside the 
> class with enumerators.
> 
> Just a suggestion, feel free to ignore.
Yeah, the downside to that is that it *does* pollute the clangd:: namespace. So 
both are a bit sad.

This header is fairly widely visible (since it's included by clangdserver) and 
this API is fairly narrowly interesting, so as far as completion goes I think I 
prefer it being hidden in TUScheduler.


Repository:
  rL LLVM

https://reviews.llvm.org/D51438



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

[PATCH] D51504: [clangd] Flatten out Symbol::Details. It was ill-conceived, sorry.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51504

Files:
  clangd/CodeComplete.cpp
  clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Merge.cpp
  clangd/index/Merge.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolYAML.cpp
  clangd/index/SymbolYAML.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -39,22 +39,16 @@
 MATCHER_P(Labeled, Label, "") {
   return (arg.Name + arg.Signature).str() == Label;
 }
-MATCHER(HasReturnType, "") {
-  return arg.Detail && !arg.Detail->ReturnType.empty();
-}
-MATCHER_P(ReturnType, D, "") {
-  return arg.Detail && arg.Detail->ReturnType == D;
-}
-MATCHER_P(Doc, D, "") { return arg.Detail && arg.Detail->Documentation == D; }
+MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
+MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
+MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
 MATCHER_P(Snippet, S, "") {
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
 MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
-MATCHER_P(IncludeHeader, P, "") {
-  return arg.Detail && arg.Detail->IncludeHeader == P;
-}
+MATCHER_P(IncludeHeader, P, "") { return arg.IncludeHeader == P; }
 MATCHER_P(DeclRange, Pos, "") {
   return std::tie(arg.CanonicalDeclaration.Start.Line,
   arg.CanonicalDeclaration.Start.Column,
@@ -696,9 +690,8 @@
 Line: 1
 Column: 1
 IsIndexedForCodeCompletion:true
-Detail:
-  Documentation:'Foo doc'
-  ReturnType:'int'
+Documentation:'Foo doc'
+ReturnType:'int'
 ...
 )";
   const std::string YAML2 = R"(
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -198,46 +198,37 @@
   R.References = 2;
   L.Signature = "()";   // present in left only
   R.CompletionSnippetSuffix = "{$1:0}"; // present in right only
-  Symbol::Details DetL, DetR;
-  DetL.ReturnType = "DetL";
-  DetR.ReturnType = "DetR";
-  DetR.Documentation = "--doc--";
-  L.Detail = &DetL;
-  R.Detail = &DetR;
+  R.Documentation = "--doc--";
   L.Origin = SymbolOrigin::Dynamic;
   R.Origin = SymbolOrigin::Static;
 
-  Symbol::Details Scratch;
-  Symbol M = mergeSymbol(L, R, &Scratch);
+  Symbol M = mergeSymbol(L, R);
   EXPECT_EQ(M.Name, "Foo");
   EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:///left.h");
   EXPECT_EQ(M.References, 3u);
   EXPECT_EQ(M.Signature, "()");
   EXPECT_EQ(M.CompletionSnippetSuffix, "{$1:0}");
-  ASSERT_TRUE(M.Detail);
-  EXPECT_EQ(M.Detail->ReturnType, "DetL");
-  EXPECT_EQ(M.Detail->Documentation, "--doc--");
+  EXPECT_EQ(M.Documentation, "--doc--");
   EXPECT_EQ(M.Origin,
 SymbolOrigin::Dynamic | SymbolOrigin::Static | SymbolOrigin::Merge);
 }
 
 TEST(MergeTest, PreferSymbolWithDefn) {
   Symbol L, R;
-  Symbol::Details Scratch;
 
   L.ID = R.ID = SymbolID("hello");
   L.CanonicalDeclaration.FileURI = "file:/left.h";
   R.CanonicalDeclaration.FileURI = "file:/right.h";
   L.Name = "left";
   R.Name = "right";
 
-  Symbol M = mergeSymbol(L, R, &Scratch);
+  Symbol M = mergeSymbol(L, R);
   EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/left.h");
   EXPECT_EQ(M.Definition.FileURI, "");
   EXPECT_EQ(M.Name, "left");
 
   R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored.
-  M = mergeSymbol(L, R, &Scratch);
+  M = mergeSymbol(L, R);
   EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h");
   EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp");
   EXPECT_EQ(M.Name, "right");
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -177,7 +177,7 @@
   Req.Query = "";
   bool SeenSymbol = false;
   M.fuzzyFind(Req, [&](const Symbol &Sym) {
-EXPECT_TRUE(Sym.Detail->IncludeHeader.empty());
+EXPECT_TRUE(Sym.IncludeHeader.empty());
 SeenSymbol = true;
   });
   EXPECT_TRUE(SeenSymbol);
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -564,12 +564,10 @@
 
   IgnoreDiagnostics DiagConsumer;