[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks! The only major comment I have is about the `FixItsScore`, others are 
mostly NITs.




Comment at: clangd/Quality.cpp:298
+FixItCount += FixIt.CodeToInsert.size();
+  }
 }

NIT: remove braces around single-statement loop body



Comment at: clangd/Quality.h:80
   bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private).
+  int FixItCount = 0; // Number of additional changes that needs to be 
performed
+  // for this change to take place.

The comment is a bit misleading, since it's actually the length of the inserted 
code.
I'd suggest keeping the comment and a name of this variable a bit more vague on 
the definition of this field, e.g. something like

`int FixItScore; // Measure of the total amount of edit needed by the fix-its. 
Higher is worse, i.e. more code needs to change`

And overall, it looks like the penalty might be too harsh for some potential 
fix-its. E.g. imagine a fix-it that adds a `using ns::some_class` directive 
somewhere to the file. The size of the inserted text will be proprotional to 
the size of the fully-qualfiied-name of the class, but it does not seem like 
the fix-its for items with larger names should be downranked more than others, 
after all it's the same type of fix-it.

```
// Hypothetical example, we don't have fix-its like that yet.
namespace ns {
  class SomeVeryLongName {};
  class ShortName {};
}


^ // <-- complete here, imagine the completion items for both classes are 
available and 
  // add `using ns::;`  to the global namespace.
```

In that case, we will penalize `SomeVeryLongName` more than `ShortName`, 
because it has a longer fix-it, but it does not look like a useful distinction.
I suggest we start with something simple, like having the same predefined 
penalty for all fix-its or penalize based on the number of fix-its in the list. 
We can generalize later when we see more fix-its and have a better 
understanding on penalties we want from them



Comment at: unittests/clangd/CodeCompleteTests.cpp:112
+
+CodeCompleteResult completions(ClangdServer &Server, Annotations Test,
+   std::vector IndexSymbols = {},

NIT: use `const Annotations&`?



Comment at: unittests/clangd/CodeCompleteTests.cpp:119
+
+CodeCompleteResult completions(ClangdServer &Server, StringRef Text,
+   std::vector IndexSymbols = {},

The number of `completions` overloads seems to be getting out of hand.
Any ideas on how we can reduce them?

Generally, I think we want the most general version:
```
CodeCompleteResult completionsImpl(ClangdServer&, Text, Point, Index, Opts)
```
And two conveniece wrappers that create `ClangdServer` for us:
```
// Parses Annotations(Text) and creates a ClangdServer instance.
CodeCompleteResult completions(Text, Index = ..., Opts = ...);
// Only creates a ClangdServer instance.
CodeCompleteResult completions(Text, Point, Index=..., Opts =...);
```

Can we make the rest of the code to use this API? (e.g. if function accepts a 
`ClangdServer`, the callers have to do quite a bit of setup anyway and I don't 
think adding the annotations parsing to get the completion point will hurt the 
readability  there). 



Comment at: unittests/clangd/CodeCompleteTests.cpp:1397
+EXPECT_TRUE(C.FixIts.size() == 1u || C.Name == "AuxFunction");
+if (!C.FixIts.empty()) {
+  EXPECT_THAT(C.FixIts, ElementsAre(ReplacementEdit));

NIT: remove braces around single-statement ifs, LLVM code generally tends to 
avoid adding braces around single-statement bodies of loops, conditionals, etc



Comment at: unittests/clangd/CodeCompleteTests.cpp:1439
+  CodeCompleteOptions Opts;
+  Opts.IncludeFixIts = true;
+  StringRef TestCode(

We tend to unit-test the scores separately from general completion tests.
Could we replace this test with a unit-test in `QualityTests.cpp`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50193



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

[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, with a few NITs.
Thanks for the change!




Comment at: clangd/Quality.h:81
+  bool NeedsFixIts =
+  false; // Whether fixits needs to be applied for that completion or not.
 

NIT: put the comment at the previous line to keep `false` at the same line.
NIT2: use three slashes, to make it a doxygen comment (previous one should also 
be three slashes).



Comment at: unittests/clangd/CodeCompleteTests.cpp:99
+
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.

It seems this comment got moved accidentally, move it back?



Comment at: unittests/clangd/QualityTests.cpp:360
+  RelevanceWithFixIt.merge(
+  CodeCompletionResult(&findDecl(AST, "x"), 0, nullptr, false, true, 
{{}}));
+  EXPECT_TRUE(RelevanceWithFixIt.NeedsFixIts);

NIT: took me some time to figure out the difference between this and the next 
one.
Maybe add the explicit type name to make more implicit that we're creating a 
fix-it here, i.e. `{FixItHint{}}`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50193



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

+1 Sam's suggestion of configuring this during initial LSP handshake, rather 
than as a command-line flag.
We could put it into `ClientCapabilities` or into `initializationOptions`. The 
latter has type 'any' in LSP, so it seems to be most in-line with the protocol 
to put clangd-specific config options there, but not opposed to having this in 
other structs as an extension either.


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] D50154: [clangd] capitalize diagnostic messages

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote:

> What's the motivation for clangd to differ from clang here?


The presentation of diagnostics to the users is different between clang and 
clangd:

- clang diagnostics are always prefixed with the file, location and severity of 
the diagnostic, e.g. `main.cpp:1:2: error: expected '}'`
- In LSP clients (VSCode in particular), the diagnostic message is the first 
thing the user sees and it looks a little nicer (subjectively) if the first 
letter is capitalized.

See the screenshots from VSCode on how diagnostics are presented:
F6901986: image.png  F6901990: image.png 



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



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

[PATCH] D50443: [clang] Store code completion token range in preprocessor.This change is to support a new fature in clangd, tests will be send toclang-tools-extra with that change.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Maybe split the commit message into multiple lines?

I suggest we wait for clangd changes to be reviewed and land them together, so 
that we have the tests and usages of this code.
Other than that LG




Comment at: include/clang/Lex/Preprocessor.h:1139
+  /// on.
+  void setCodeCompletionTokenRange(const SourceLocation start,
+   const SourceLocation end) {

NIT: parameters are are `UpperCamelCase` in LLVM naming conventions


Repository:
  rC Clang

https://reviews.llvm.org/D50443



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

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Short summary of the offline discussion: I suggest adding this parameter into 
the corresponding requests of the index (FuzzyFindRequest, 
FindDefinitionsRequest) instead of stashing it in the context. Context has all 
the same problems as the global variables, so we should try to avoid using it 
where possible.
On the other hand, where this key is stored might not be terribly important if 
we don't have too many usages and remove it when we can workaround the 
limitations that we're currently facing.

In any case, we should add a comment that the active file should be avoided and 
will be removed.




Comment at: clangd/ClangdServer.cpp:162
 
+WithContextValue WithFileName(kActiveFile, File);
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check

If we want to set this value more consistently, maybe do it in 
TUScheduler::runWithPreamble/runWithAST? 
All interesting operations that work on files go through those, so it's easier 
to certify that the file is set correctly.



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's 
supposed to be used to workaround current limitations?
`Context.h/cpp` is a general-purpose support library for the contexts, 
application-specific keys do not fit in well there.



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

ilya-biryukov wrote:
> Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's 
> supposed to be used to workaround current limitations?
> `Context.h/cpp` is a general-purpose support library for the contexts, 
> application-specific keys do not fit in well there.
Maybe use `std::string`? Contexts can easily be cloned/migrated between 
threads, so it's very easy to end up with pointers to dead strings here.



Comment at: clangd/Context.h:223
+// individual requests.
+extern const clang::clangd::Key kActiveFile;
+

Maybe choose a scarier name and add a comment that we're not gonna use it in 
the long-term?
Maybe also add a comment that this is only used in our index implementation to 
workaround the limitations of the current design? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446



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

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:162
 
+WithContextValue WithFileName(kActiveFile, File);
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check

ioeric wrote:
> ilya-biryukov wrote:
> > If we want to set this value more consistently, maybe do it in 
> > TUScheduler::runWithPreamble/runWithAST? 
> > All interesting operations that work on files go through those, so it's 
> > easier to certify that the file is set correctly.
> I have two concerns about doing this:
> 1. Setting it in the TUScheduler seems to bury this deeper e.g. into 
> scheduling logic. Limiting this in specific callbacks right before it's 
> needed seems to make it less accessible.
> 
> 2. There are operations like `workspaceSymbols` (although not handled in this 
> patch)  that do not go through TUScheduler.
> 1. Setting it in the TUScheduler seems to bury this deeper e.g. into 
> scheduling logic. Limiting this in specific callbacks right before it's 
> needed seems to make it less accessible.

If we want to make the data-flow explicit, let's add this to the index request 
parameters.
Otherwise, I suggest we do something consistent across all requests, rather 
than special-casing the index-calling ones. E.g. if someone adds an index call 
in `onHover`, they'll need to remember to also update the ClangdServer call to 
set this value and they really shouldn't care about this hack of ours. The less 
intrusive this thing is, the better.

> 2. There are operations like workspaceSymbols (although not handled in this 
> patch) that do not go through TUScheduler.
And we don't set the file path for them anyway, because there's not active file 
in that case. How is that different in either of the approaches?



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

ioeric wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > Maybe move it somewhere else? E.g. to `Index.h`, since that's where 
> > > > it's supposed to be used to workaround current limitations?
> > > > `Context.h/cpp` is a general-purpose support library for the contexts, 
> > > > application-specific keys do not fit in well there.
> > > Maybe use `std::string`? Contexts can easily be cloned/migrated between 
> > > threads, so it's very easy to end up with pointers to dead strings here.
> > I intentionally used `StringRef` here to make it harder to use and hoped 
> > that users would be more careful when using this. For the current simple 
> > use cases, `StringRef` should be sufficient IMO.
> We should probably look for some other place to put this, but I don't think 
> `Index.h` is the right place. Although context might not be the best place to 
> host the active file, the active file seems to be a well-defined concept in 
> clangd in general and not index-specific.
Maybe move it `ClangdServer.h` or `TUScheduler,h` then? (depending on where we 
set it).

I still don't think `Context.h` is the right place for it,  it should not 
contain any application-specific keys.



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

ilya-biryukov wrote:
> ioeric wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ilya-biryukov wrote:
> > > > > Maybe move it somewhere else? E.g. to `Index.h`, since that's where 
> > > > > it's supposed to be used to workaround current limitations?
> > > > > `Context.h/cpp` is a general-purpose support library for the 
> > > > > contexts, application-specific keys do not fit in well there.
> > > > Maybe use `std::string`? Contexts can easily be cloned/migrated between 
> > > > threads, so it's very easy to end up with pointers to dead strings here.
> > > I intentionally used `StringRef` here to make it harder to use and hoped 
> > > that users would be more careful when using this. For the current simple 
> > > use cases, `StringRef` should be sufficient IMO.
> > We should probably look for some other place to put this, but I don't think 
> > `Index.h` is the right place. Although context might not be the best place 
> > to host the active file, the active file seems to be a well-defined concept 
> > in clangd in general and not index-specific.
> Maybe move it `ClangdServer.h` or `TUScheduler,h` then? (depending on where 
> we set it).
> 
> I still don't think `Context.h` is the right place for it,  it should not 
> contain any application-specific keys.
We're relying too much on the data flow of contexts, which is implicit and we 
completely don't control. Even if we don't access dead StringRefs now, it's 
very easy to accidentally introduce this behavior later.

We should use `string` here, there's just too high risk of hitting a bug 
otherwise. What are the disadvantages of using `string` here?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446



___

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The new behavior looks reasonable for interactive tools like clangd, but I'm a 
bit worried that clang may emit too many irrelevant errors, because it may not 
cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that 
mode.
Nevertheless, I think this is moving clangd in the right direction and if we 
see too many irrelevant errors, we should look into improving clang to show 
less of those.




Comment at: test/clangd/missing-includes.test:1
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

This tests looks useful, but could we also unit-test (with a C++ gtest) that 
fatal errors do not block other errors when:
1. building the preamble (i.e. when calling `buildPreamble`),
2. building the AST (i.e. when calling `ParsedAST::build`).

To have a regression test for potential changes in those areas.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:441
  const clangd::CodeCompleteOptions &CCOpts,
+ const ClangdDiagnosticOptions &DiagOpts,
  llvm::Optional CompileCommandsDir,

Maybe remove this parameter now that we configure these via LSP?
We don't set it to anything other than the default anyway.


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] D50446: [clangd] Record the file being processed in a TUScheduler thread in context.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!




Comment at: clangd/TUScheduler.h:145
 };
+
 } // namespace clangd

NIT: accidental whitespace change?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446



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

[PATCH] D50449: [clangd] Support textEdit in addition to insertText.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:289
   }
+  std::stable_sort(Completion.FixIts.begin(), Completion.FixIts.end(),
+   [](const TextEdit &X, const TextEdit &Y) {

We shouldn't have duplicate/overlapping fix-its, right? Maybe use `std::sort`?



Comment at: clangd/CodeComplete.cpp:291
+   [](const TextEdit &X, const TextEdit &Y) {
+ return X.range.start.line < Y.range.start.line ||
+X.range.start.character <

use built-in tuples comparison `std::tie(X.line, X.column) < std::tie(Y.line, 
Y.column)`?



Comment at: clangd/CodeComplete.cpp:1057
+Range TextEditRange;
+if (CodeCompletionRange.isValid()) {
+  TextEditRange = halfOpenToRange(Recorder->CCSema->getSourceManager(),

Maybe add a comment describing the cases where `isValid()` is false?
Does it happen when we complete with empty identifier?



Comment at: clangd/CodeComplete.cpp:1310
+  // other.
+  for (const auto &FixIt : FixIts) {
+if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {

Maybe keep the `reserve` call? (we could reserve one extra element, but that's 
fine)



Comment at: clangd/CodeComplete.h:126
 
+  TextEdit textEdit;
+

Could we avoid adding `textEdit` here?
It's an intermediate representation that is only used in clangd, and we should 
be able materialize the actual `textEdit` when converting to LSP.



Comment at: unittests/clangd/SourceCodeTests.cpp:40
 
+Range range(const std::pair p1,
+const std::pair p2) {

we convert those `uint64_t` into `int` when setting the positions anyway.
Maybe use `int` here too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50449



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I have left a few comments, but wanted to start with a higher-level design 
consideration.

I believe we should probably expose the cancellations in the ClangdServer API 
directly.
The reasons for that are:

1. We have an internal client that would also benefit from using the 
cancellation API and we should try to share as much code as possible between 
that and the rest of clangd.  Creating the cancellation cookies, stashing them 
in the context are all details that could be handled by ClangdServer so two 
clients don't have to do the same things.
2. `ClangdServer` seems like a natural layer for adding this, since it's a 
user-facing **async** API. It spawns async tasks, so it is a perfect place to 
provide a way to cancel the spawned tasks as well.

The concrete API that I propose is:

- Relevant methods of ClangdServer (we could start we code completion) that 
spawn async reqeuests returns a cancellation object that allows to cancel the 
spawned job:

  class TaskHandle {
void cancel();
  };
  
  class ClangdServer {
TaskHandle codeComplete(File, Position, Callback);
  };

- Users of the API are free to ignore the `TaskHandle` if they don't need to 
cancel the corresponding requests.
- If the users are capable of cancelling the request, they are responsible for 
keeping the `TaskHandle` around before the corresponding callback is called.
- If the request was cancelled, the `Callback` might receive the 
`TaskCancelledError` in the callback and should handle it accordingly.

The rest of the design LG, i.e.  the code that can actually be cancelled is 
responsible for checking `CancellationHandler::HasCancelled` and should return 
a corresponding error in the callback if it was cancelled.




Comment at: clangd/Cancellation.h:1
+//===--- Cancellation.h 
---*-C++-*-===//
+//

This file could use some comments about the API and samples of how it can be 
used.
Could probably be done after we finalize the design, though.



Comment at: clangd/Cancellation.h:22
+public:
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(

I think we might want to expose the context key for the cancellation primitive 
here.
The reason is that checking if request is cancelled is a common operation (i.e. 
I can imagine it being called on every few parsed ast node, etc.), so we'd want 
to keep the overhead of checking for cancellation very low.

Exposing a key in the context would allow to avoid doing the lookups in the 
context every time we need to check if request was cancelled.

Note that exposing `shared_ptr>` is probably not a great idea, 
since it'll allow to write into that object too. I suggest we go with a class 
that wraps it and only allows to read the value of the variable, i.e. only has 
`isCancelled` method.



Comment at: clangd/Cancellation.h:23
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(
+  std::shared_ptr> CancellationToken);

The `LLVM_NODISCARD` is misplaced. The current code applies the attribute to 
the type (i.e. `Context`), and we want to apply to the function.
There are two ways to do that:
1. `LLVM_NODISCARD static Context SetCurrentCancellationToken()`
2. `static Context SetCurrentCancellationToken LLVM_NODISCARD ()`

The first one is definitely less awkward and more widely used, so I suggest we 
stick to it



Comment at: clangd/Cancellation.h:25
+  std::shared_ptr> CancellationToken);
+  static llvm::Error GetCancellationError();
+};

NIT: use `lowerCamelCase` for function names (per [LLVM style 
guide](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly))



Comment at: clangd/Cancellation.h:36
+  std::error_code convertToErrorCode() const override {
+llvm_unreachable("Tried to get error code on TaskCancelledError");
+  }

It seems the contract for `ErrorInfo` actually requires this function to be 
implemented, so using `llvm_unreachable` doesn't seem right here.
We could get away without actually unique error codes by returning 
`llvm::inconvertibleErrorCode()`, though.



Comment at: clangd/ClangdLSPServer.cpp:76
+  return ID.kind() == json::Value::Number
+ ? utostr(static_cast(ID.getAsNumber().getValue()))
+ : std::string(ID.getAsString().getValue());

1. Maybe use `getAsInteger()` to convert directly to `int64_t`?
2. Maybe use `itostr` to avoid conversion from `int64` to `uint64`, which is 
undefined for negative numbers?
3. What if input is neither string nor number? Maybe add an assertion and check 
at the call-sites? That's a form of user input and we don't want clangd to 
crash in case of incorrect ones.

Alternatively, could we simply pipe the `json::Value`

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Only a few NITs from my side.
Excited for this fix to get in, have been seeing duplicate in other cases too 
:-)




Comment at: clangd/SourceCode.h:69
 
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager &SourceMgr);

This function looks like a good default choice for normalizing paths before 
putting them into LSP structs, ClangdServer responses, etc.
I suggest we add a small comment here with a guideline for everyone to attempt 
using it whenever possible. WDYT?



Comment at: unittests/clangd/TestFS.h:43
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.

s/not null/not empty



Comment at: unittests/clangd/TestFS.h:46
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.

s/not null/not empty


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM with a small NIT.




Comment at: clangd/ClangdLSPServer.cpp:507
+}
+LSPDiag["clangd.fixes"] = std::move(ClangdFixes);
+  }

NIT: maybe avoid `.` and use something like `clangd_fixes`? having dots in the 
object keys might be awkward if someone tries to use those from javascript. 
(not that we have these use-cases now, but still)


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] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:715
  unsigned NumCandidates) override {
+TopN Top(
+std::numeric_limits::max());

Maybe use `vector`, followed by `std::sort` at the end?

Or is there any functionality that we can reuse from the priority queue?



Comment at: clangd/Quality.h:170
+  bool ContainsActiveParameter = false;
+  CodeCompleteConsumer::OverloadCandidate::CandidateKind Kind;
+

Maybe set some default value to avoid UB if someone forgets to set it?



Comment at: clangd/Quality.h:172
+
+  float evaluate() const;
+};

Maybe write a direct comparison here instead of `evaluate`?
Having floating scores makes sense for completion, where lot's of different 
signals from multiple sources are taken into account and clients have to 
rescore on the client side.
Not so much for SignatureHelp, where we have all required information to 
compare the signatures is on clangd side and clients won't need to rescore.

A comparison operator should be easier to reason about, though.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50555



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

[PATCH] D50449: [clangd] Support textEdit in addition to insertText.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but 
our hacked-up ycm integration seems to work, right?)




Comment at: clangd/CodeComplete.cpp:1310
+  // other.
+  for (const auto &FixIt : FixIts) {
+if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {

kadircet wrote:
> ilya-biryukov wrote:
> > Maybe keep the `reserve` call? (we could reserve one extra element, but 
> > that's fine)
> Actually we could have much more than one extra element, not for the current 
> situation but may be in the future when we have more fixit completions.
We can't have more than one edit adjacent to the completion identifier, right? 
Otherwise they'll conflict.
It is possible to have multiple edits which are consectutive and the last of 
them is adjacent to the completion identifier, though. But I don't think we 
handle those cases here anyway.

But I'm not too worried about leaving out the reserve call either. At the very 
worst we could waste some memory on a single completion item, but we shouldn't 
keep too many around at the same time anyway, so feel free to ignore this one.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50449



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

[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM with a few NITs




Comment at: clangd/CodeComplete.cpp:687
+struct ScoredSignatureGreater {
+  bool operator()(const ScoredSignature &L, const ScoredSignature &R) {
+// Ordering follows:

NIT: Maybe make it a function with a descriptive name, e.g. 
`hasBetterSignature`?
We could call it in a lambda, should make the code even clearer.



Comment at: clangd/CodeComplete.cpp:755
+  ScoredSignatureGreater());
+for (const auto &SS : ScoredSignatures) {
+  SigHelp.signatures.push_back(SS.second);

NIT: remove braces around the single-statement loop body


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50555



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

[PATCH] D50443: [clang] Store code completion token range in preprocessor.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, but let's land together with a dependent revision to hove some code that 
actually tests it.




Comment at: include/clang/Lex/Preprocessor.h:313
 
+  /// Range for the code completion taken.
+  SourceRange CodeCompletionTokenRange;

NIT: s/taken/token


Repository:
  rC Clang

https://reviews.llvm.org/D50443



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

[PATCH] D50443: [clang] Store code completion token range in preprocessor.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

NIT: maybe also note the number of the clangd revision in this change's 
description


Repository:
  rC Clang

https://reviews.llvm.org/D50443



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

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Diagnostics.h:40
   DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;
+  unsigned Category;
   // Since File is only descriptive, we store a separate flag to distinguish

Maybe store the string name of the category directly here? This struct was 
never optimized for efficiency, but aims to be independent of clang, so that 
the clients might interpret it without calling any clang-specific code (we do 
reuse the `Level` enum, though, but that's merely for code reuse)



Comment at: clangd/Protocol.h:549
+  /// The diagnostic's category. Can be omitted.
+  /// An LSP extension that's used to send the name of the category over to the
+  /// client.

Maybe add a brief example of category names here, i.e. "semantic issue", etc.?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Maybe also add a test for find-definition that was broken before? (non-empty 
preamble -> empty preamble -> bad gotodef that goes to included file instead of 
the local variable)
To have a regression test against similar failures.




Comment at: unittests/clangd/TUSchedulerTests.cpp:312
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,

Maybe call PrecompiledPreamble directly? it requires some setup, but tests the 
modified code without extra layers that TUScheduler adds.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627



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

[PATCH] D50628: [Preamble] Empty preamble is not an error.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D50628



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

[PATCH] D50438: [clangd] Sort GoToDefinition results.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/XRefs.cpp:71
 
+struct DeclInfo {
+  const Decl *D;

NIT: maybe call `Occurence` instead? As this is actually a `Decl` with some 
extra data, computed based on the expression it originated from. Occurence 
seems like a nice that for that kind of thing.



Comment at: clangd/XRefs.cpp:105
+// Sort results. Declarations being referenced explicitly come first.
+std::sort(Result.begin(), Result.end(),
+  [](const DeclInfo &L, const DeclInfo &R) {

Maybe sort further at the callers instead?
It would be a more intrusive change, but would also give a well-defined result 
order for findDefinitions and other cases. E.g. findDefinitions currently gives 
results in an arbitrary order (specifically, the one supplied by DenseMap+sort) 
when multiple decls are returned.
WDYT?



Comment at: clangd/XRefs.cpp:128
+  void insertDecl(const Decl *D, bool IsExplicitReferenced) {
+auto It = Decls.find(D);
+if (It != Decls.end())

Maybe simplify to `Decls[D] |= IsExplicitReferenced`?



Comment at: clangd/XRefs.cpp:143
+  auto hasImplicitExpr = [](const Expr *E) {
+if (E && E->child_begin() != E->child_end()) {
+  // Use the first child is good enough for most cases -- normally the

NIT: [use early 
exits](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)
 by inverting condition


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50438



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

[PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, kadircet.
Herald added subscribers: arphaman, jkorous, MaskRay.

To avoid producing very verbose output in substitutions involving
typedefs, e.g.

  T -> std::vector::iterator

gets turned into an unreadable mess when printed out for libstdc++,
result contains internal types (std::__Vector_iterator<...>) and
expanded well-defined typedefs (std::basic_string).

Until we improve the presentation code in clang, going with
non-instantiated decls looks like a better UX trade-off.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50645

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1537,6 +1537,59 @@
   EXPECT_EQ(0, Results.activeParameter);
 }
 
+TEST(SignatureHelpTest, InstantiatedSignatures) {
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(10, ^);
+})cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T...);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T...) -> void", {"T..."})));
+
+  // It is debatable whether we should substitute the outer template parameter
+  // ('T') in that case. Currently we don't substitute it in signature help, 
but
+  // do substitute in code complete.
+  // FIXME: make code complete and signature help consistent, figure out which
+  // way is better.
+  EXPECT_THAT(signatures(R"cpp(
+template 
+struct X {
+  template 
+  void foo(T, U);
+};
+
+int main() {
+  X().foo(^)
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -714,7 +714,15 @@
"too many arguments");
 SigHelp.activeParameter = static_cast(CurrentArg);
 for (unsigned I = 0; I < NumCandidates; ++I) {
-  const auto &Candidate = Candidates[I];
+  OverloadCandidate Candidate = Candidates[I];
+  // We want to avoid showing instantiated signatures, because they may be
+  // long in some cases (e.g. when 'T' is substituted with 'std::string', 
we
+  // would get 'std::basic_string').
+  if (auto Func = Candidate.getFunction()) {
+if (auto Pattern = Func->getTemplateInstantiationPattern())
+  Candidate = OverloadCandidate(Pattern);
+  }
+
   const auto *CCS = Candidate.CreateSignatureString(
   CurrentArg, S, *Allocator, CCTUInfo, true);
   assert(CCS && "Expected the CodeCompletionString to be non-null");


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1537,6 +1537,59 @@
   EXPECT_EQ(0, Results.activeParameter);
 }
 
+TEST(SignatureHelpTest, InstantiatedSignatures) {
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(10, ^);
+})cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T...);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T...) -> void", {"T..."})));
+
+  // It is debatable whether we should substitute the outer template parameter
+  // ('T') in that case. Currently we don't substitute it in signature help, but
+  // do substitute in code complete.
+  // FIXME: make code complete and signature help consistent, figure out which
+  // way is better.
+  EXPECT_THAT(signatures(R"cpp(
+template 
+struct X {
+  template 
+  void foo(T, U);
+};
+
+int main() {
+  X().foo(^)
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd

[PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 160536.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Use 'auto*' instead of 'auto'


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50645

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1537,6 +1537,59 @@
   EXPECT_EQ(0, Results.activeParameter);
 }
 
+TEST(SignatureHelpTest, InstantiatedSignatures) {
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(10, ^);
+})cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T...);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T...) -> void", {"T..."})));
+
+  // It is debatable whether we should substitute the outer template parameter
+  // ('T') in that case. Currently we don't substitute it in signature help, 
but
+  // do substitute in code complete.
+  // FIXME: make code complete and signature help consistent, figure out which
+  // way is better.
+  EXPECT_THAT(signatures(R"cpp(
+template 
+struct X {
+  template 
+  void foo(T, U);
+};
+
+int main() {
+  X().foo(^)
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -714,7 +714,15 @@
"too many arguments");
 SigHelp.activeParameter = static_cast(CurrentArg);
 for (unsigned I = 0; I < NumCandidates; ++I) {
-  const auto &Candidate = Candidates[I];
+  OverloadCandidate Candidate = Candidates[I];
+  // We want to avoid showing instantiated signatures, because they may be
+  // long in some cases (e.g. when 'T' is substituted with 'std::string', 
we
+  // would get 'std::basic_string').
+  if (auto *Func = Candidate.getFunction()) {
+if (auto *Pattern = Func->getTemplateInstantiationPattern())
+  Candidate = OverloadCandidate(Pattern);
+  }
+
   const auto *CCS = Candidate.CreateSignatureString(
   CurrentArg, S, *Allocator, CCTUInfo, true);
   assert(CCS && "Expected the CodeCompletionString to be non-null");


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1537,6 +1537,59 @@
   EXPECT_EQ(0, Results.activeParameter);
 }
 
+TEST(SignatureHelpTest, InstantiatedSignatures) {
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T, T, T);
+
+int main() {
+  foo(10, ^);
+})cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
+
+  EXPECT_THAT(signatures(R"cpp(
+template 
+void foo(T...);
+
+int main() {
+  foo(^);
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T...) -> void", {"T..."})));
+
+  // It is debatable whether we should substitute the outer template parameter
+  // ('T') in that case. Currently we don't substitute it in signature help, but
+  // do substitute in code complete.
+  // FIXME: make code complete and signature help consistent, figure out which
+  // way is better.
+  EXPECT_THAT(signatures(R"cpp(
+template 
+struct X {
+  template 
+  void foo(T, U);
+};
+
+int main() {
+  X().foo(^)
+}
+  )cpp")
+  .signatures,
+  ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -714,7 +714,15 @@
"too many arguments");
 SigHelp.activeParameter = static_cast(CurrentArg);
 for (unsigned I = 0; I < NumCandidates; ++I) {
-  const auto &Candidate = Candidates[I];
+  OverloadCandidate Candidate = Candidate

[PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:721
+  // would get 'std::basic_string').
+  if (auto Func = Candidate.getFunction()) {
+if (auto Pattern = Func->getTemplateInstantiationPattern())

hokein wrote:
> nit: auto*, the same below.
Good catch, thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50645



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for missing this one. The biggest concern that I have is about the 
thread-safety of the API, it's too easy to misuse from multiple threads at this 
point.

We should define thread-safety guarantees for `TaskHandle` and 
`CancellationToken` and make sure they're not easily broken. The suggested way 
is to make both move-only types.
For `CancellationToken` we might need a non-thread-safe copy operation, though. 
But maybe make it a separate method (`clone`?) with a clear statement that it's 
not thread-safe?

Other than that just NITs.




Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key>> CancellationTokenKey;
+} // namespace

Having a `shared_ptr` key in the Context can cause data races (e.g. if we copy 
it concurrently from multiple threads).
I suggest we make `CancellationToken` move-only (i.e. disallow copies) and 
return `const CancellationToken&` when getting it from the context.



Comment at: clangd/Cancellation.cpp:34
+llvm::Error CancellationHandler::getCancellationError() {
+  return llvm::make_error();
+}

I'm not sure if this function buys us much in terms of clarity, maybe remove it 
and inline everywhere?



Comment at: clangd/Cancellation.h:9
+//===--===//
+// CancellationToken mechanism for async threads. The caller can generate a
+// TaskHandle for cancellable tasks, then bind that handle to current context

The comments seems to mention all the important bits, but maybe we could 
separate different concerns more clearly based on how the code should be used?
E.g. here's a rough draft to illustrate the idea:

```
Cancellation mechanism for async tasks. Roughly all the clients of this code 
can be classified into three categories:
1. The code that creates and schedules async tasks, e.g. TUScheduler.
2. The callers of the async method that can cancel some of the running tasks, 
e.g. `ClangdLSPServer`
3. The code running inside the async task itself, i.e. code completion or find 
definition implementation that run clang, etc.

For (1), the guideline is to accept a callback for the result of async 
operation and return a `TaskHandle` to allow cancelling the request.
TaskHandle someAsyncMethod(function)> 
Callback);

The callers of async methods (2) can issue cancellations and should be prepared 
to handle `TaskCancelledError` result:
TaskHandle H = someAsyncMethod([](llvm::Expected R) {
if (/* code to check for TaskCancelledError */)
   llvm::errs() << "Task got cancelled";
 else
   llvm::errs() << "The result is: " << *R;
} 
});

sleep(5);
H.cancel();

The worker code itself (3) should check for cancellations using 
`CancellationToken` that can be retrieved via 
`CancellationToken::isCancelled()`.
void codeComplete() {
  const CancellationToken& CT = CancellationToken::isCancelled();
  if (CT.isCancelled()) 
return llvm::makeError();

  runSema();
  if (CT.isCancelled()) 
return llvm::makeError();

  queryIndex();  
  return results;
}
If the operation was cancelled before it could run to completion, it should 
propagate the TaskCancelledError as a result.
```



Comment at: clangd/Cancellation.h:60
+
+class CancellationToken {
+private:

We're missing docs on this and the other classes. I suggest a small doc 
describing what they're used for, e.g. `used for checking if the operation was 
cancelled` and `used to signal the operation should be cancelled`, etc.

Also noting the threading-safety guarantees is a probably a good idea.



Comment at: clangd/Cancellation.h:61
+class CancellationToken {
+private:
+  std::shared_ptr> Token;

NIT: move members to the end of the class to be consistent with the rest of 
clangd.



Comment at: clangd/Cancellation.h:67
+  operator bool() const { return isCancelled(); }
+  CancellationToken(const std::shared_ptr> Token)
+  : Token(Token) {}

Can we make this ctor private? 



Comment at: clangd/Cancellation.h:71
+
+class TaskHandle {
+public:

I wonder if we should make `TaskHandle` move-only?

The reasoning is that is can be easily used from multiple threads (since it's 
used by code dealing with async tasks anyway) and copying it concurrently is a 
data race.
On the other hand, calling `cancel()` is perfectly thread-safe and shouldn't 
cause any problems.



Comment at: clangd/Cancellation.h:74
+  void cancel();
+  static TaskHandle createCancellableTaskHandle();
+  friend class CancellationHandler;

NIT: maybe use a shorter name, e.g. `TaskHandle::create`?



Comment at: clangd/Cancellation.h:82
+
+class CancellationHandler {
+public:

[PATCH] D50641: [clangd][test] Fix exit messages in tests

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Many thanks for fixing this.

Adding some failure monitoring seems like a nice idea. On the other hand, 
polluting every test with stderr redirection doesn't look like a nice idea.
As a middle ground, I could imagine some extra checking being done if 
`-lit-test` is passed, e.g. returning a non-zero error code on request parsing 
errors would trigger test failures in these particular cases, right?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50641



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

[PATCH] D50641: [clangd][test] Fix exit messages in tests

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting 
with failure error code


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50641



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

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM.  Let's watch out for possible breakages in any of the clients, though. 
I've checked VSCode and it seems to be fine with the added fields.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, kadircet.
Herald added subscribers: arphaman, jkorous, MaskRay.

Previously, clangd was trying to show documentation for the active
parameter instead, which is wrong per LSP specification.

Moreover, the code path that attempts to get parameter comments never
succeds, because no attempt is made to parse function doc comment and
extract parameter-specific parts out of it. So we also remove the code
that claims to fetch parameter comments: it is not used anymore and is
incorrect.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726

Files:
  clangd/CodeComplete.cpp
  clangd/CodeCompletionStrings.cpp
  clangd/CodeCompletionStrings.h

Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,10 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will be returned. It is used to workaround crashes when parsing
-/// comments in the stale headers, coming from completion preamble.
 std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders);
+getDocComment(const ASTContext &Ctx,
+  const CodeCompleteConsumer::OverloadCandidate &Overload,
+  bool CommentsFromHeaders);
 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector::at(size_type) const, this returns:
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -41,28 +41,17 @@
   return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
 }
 
-} // namespace
-
-std::string getDocComment(const ASTContext &Ctx,
-  const CodeCompletionResult &Result,
-  bool CommentsFromHeaders) {
-  // FIXME: clang's completion also returns documentation for RK_Pattern if they
-  // contain a pattern for ObjC properties. Unfortunately, there is no API to
-  // get this declaration, so we don't show documentation in that case.
-  if (Result.Kind != CodeCompletionResult::RK_Declaration)
-return "";
-  auto *Decl = Result.getDeclaration();
-  if (!Decl || llvm::isa(Decl)) {
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
+  if (llvm::isa(Decl)) {
 // Namespaces often have too many redecls for any particular redecl comment
 // to be useful. Moreover, we often confuse file headers or generated
 // comments with namespace comments. Therefore we choose to just ignore
 // the comments for namespaces.
 return "";
   }
-  const RawComment *RC = getCompletionComment(Ctx, Decl);
+  const RawComment *RC = getCompletionComment(Ctx, &Decl);
   if (!RC)
 return "";
-
   // Sanity check that the comment does not come from the PCH. We choose to not
   // write them into PCH, because they are racy and slow to load.
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
@@ -72,23 +61,30 @@
   return Doc;
 }
 
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders) {
-  auto *Func = Result.getFunction();
-  if (!Func)
+} // namespace
+
+std::string getDocComment(const ASTContext &Ctx,
+  const CodeCompletionResult &Result,
+  bool CommentsFromHeaders) {
+  // FIXME: clang's completion also returns documentation for RK_Pattern if they
+  // contain a pattern for ObjC properties. Unfortunately, there is no API to
+  // get this declaration, so we don't show documentation in that case.
+  if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";
-  const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
-  if (!RC)
+  auto *Decl = Result.getDeclaration();
+  if (!Decl)
 return "";
-  // Sanity check that the comment does not come from the PCH. We choose to not
-  // write them into PCH, because they are racy and slow to load.
-  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
-  std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
-

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, kadircet.
Herald added subscribers: arphaman, mgrang, jkorous, MaskRay.

Sema can only be used for documentation in the current file, other doc
comments should be fetched from the index.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
   EXPECT_TRUE(Results.Completions.empty());
 }
 
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+  std::unique_ptr Index;
+  if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.StaticIndex = Index.get();
+
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
   return false;
   return true;
 }
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
 
 Matcher Sig(std::string Label,
   std::vector Params) {
@@ -1590,6 +1599,51 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SignatureHelpTest, IndexDocumentation) {
+  Symbol::Details DocDetails;
+  DocDetails.Documentation = "Doc from the index";
+
+  Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+  Foo0.Detail = &DocDetails;
+  Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+  Foo1.Detail = &DocDetails;
+  Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0, Foo1, Foo2})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+  AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -21,6 +21,8 @@
 #include 
 
 namespace clang {
+class Decl;
+
 namespace clangd {
 
 struct SymbolLocation {
@@ -60,6 +62,8 @@
 // SymbolID can be used as key in the symbol indexes to lookup the symbol.
 class SymbolID {
 public:
+  static llvm::Optional forDecl(const Decl& D);
+
   SymbolID() = default;
   explicit SymbolID(llvm::StringRef USR);
 
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "Index.h"
+#include "clang/Index/USRGeneration.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/raw_ostream.h"
@@ -23,8 +24,16 @@
 << L.End.Line << ":" << L.End.Column << ")";
 }
 
+llvm::Optional SymbolID::forDecl(const Decl &D) {
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(&D, USR))
+return llvm::None;
+  return SymbolID(USR);
+}
+
 SymbolID::SymbolID(StringRef USR)
-: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {}
+: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {
+}
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolID &ID) {
   OS << toHex(toStringRef(ID.HashValue));
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -176,7 +176,8 @@
 PrecompiledPreamble const *Preamble,
 StringRef Contents, Position Pos,
 IntrusiveRefCntPtr VFS,
-   

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

There's a test for the new behavior in https://reviews.llvm.org/D50726


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

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

- run clang-format


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
   EXPECT_TRUE(Results.Completions.empty());
 }
 
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+  std::unique_ptr Index;
+  if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.StaticIndex = Index.get();
+
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
   return false;
   return true;
 }
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
 
 Matcher Sig(std::string Label,
   std::vector Params) {
@@ -1590,6 +1599,51 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SignatureHelpTest, IndexDocumentation) {
+  Symbol::Details DocDetails;
+  DocDetails.Documentation = "Doc from the index";
+
+  Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+  Foo0.Detail = &DocDetails;
+  Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+  Foo1.Detail = &DocDetails;
+  Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0, Foo1, Foo2})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+  AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -21,6 +21,8 @@
 #include 
 
 namespace clang {
+class Decl;
+
 namespace clangd {
 
 struct SymbolLocation {
@@ -60,6 +62,8 @@
 // SymbolID can be used as key in the symbol indexes to lookup the symbol.
 class SymbolID {
 public:
+  static llvm::Optional forDecl(const Decl &D);
+
   SymbolID() = default;
   explicit SymbolID(llvm::StringRef USR);
 
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "Index.h"
+#include "clang/Index/USRGeneration.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/raw_ostream.h"
@@ -23,6 +24,13 @@
 << L.End.Line << ":" << L.End.Column << ")";
 }
 
+llvm::Optional SymbolID::forDecl(const Decl &D) {
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(&D, USR))
+return llvm::None;
+  return SymbolID(USR);
+}
+
 SymbolID::SymbolID(StringRef USR)
 : HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {}
 
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -171,12 +171,11 @@
 CodeCompleteOptions Opts);
 
 /// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand &Command,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileC

[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

+1 to having a separate option for that. More generally, wouldn't we want to 
exit on more kinds errors in the future?
JSON parsing errors is something that popped up recently, but the option only 
for that looks too narrow. We might end up wanting more options like that in 
the future, e.g. `-exit-on-json-dispatch-error`, `-exit-on-parse-error`, ...

How about a more generic option that would force clangd to have non-zero error 
code whenever even a single error was emitted via `elog()`? 
I've checked the output of `./bin/llvm-lit -avv 
../tools/clang/tools/extra/test/clangd/ | grep '\(^E\|PASS\)'` and there seems 
to be a few things that emit errors spuriously and should be fixed for that to 
work:

1. `E[10:19:11.985] Failed to get status for RootPath clangd: No such file or 
directory`.

Could be fixed by specifying a valid rootPath in all clangd tests.

2. `E[10:19:12.013] Could not build a preamble for file /clangd-test/main.cpp`.

IIUC, this happens on empty preambles, the fix is to not emit an error in that 
case. In general, whenever preamble is not built, this is probably due to 
changes in some headers and that case should be considered valid input for 
clangd, so arguably this shouldn't be an error in the first place.-

3. Some tests actually test for invalid inputs, those should probably expect an 
error code from clangd that runs into those errors.

That doesn't look like a long list, so it shouldn't be too hard. What are your 
thoughts on a more generic option like that?




Comment at: JSONRPCDispatcher.cpp:366
+if (TestMode)
+  exit(EXIT_FAILURE);
   }

Alternative would be to run input parsing to completion, propagate if any error 
was encountered to main and exit with a predefined error code in that case.
Exiting prematurely might hide some important errors that we would otherwise 
catch even before seeing an error code from clangd, e.g. infinite loops in the 
input handling on invalid inputs, etc.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50785



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

[PATCH] D50695: [clangd] Always use the latest preamble

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, we were previously getting inconsistent ASTs (i.e. using different 
preambles), depending on whether they were built in `TUScheduler::update` or in 
`TUScheduler::runWithAST` handlers.
Just a few NITs.




Comment at: clangd/TUScheduler.cpp:373
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  // Always use the NewPreamble.
+  LastBuiltPreamble = NewPreamble;

Maybe remove the comment? It does not seem to add any additional information on 
top of the code.



Comment at: unittests/clangd/XRefsTests.cpp:1058
+  // stale one.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),

Maybe use `runFindDefinitions` instead of the two `Server.findDefinitions()` + 
`Server.blockUntilIdleForTest()` calls?



Comment at: unittests/clangd/XRefsTests.cpp:1061
+  [&](llvm::Expected> Loc) {
+ASSERT_TRUE(bool(Loc));
+EXPECT_THAT(*Loc,

NIT: just use `cantFail(std::move(Loc))` to get the underlying vector and 
remove `ASSERT_TRUE`.
This gives equivalent results (crashes tests with `assert` failure internally 
in LLVM) with a simpler syntax.



Comment at: unittests/clangd/XRefsTests.cpp:1073
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),

Same here: maybe use `runFindDefinitions`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly LSP-specific comments and comments about making TaskHandle thread-safe.

I'm also starting to wonder if the scope of this patch is too big, we could 
potentially split it into three separate bits:

1. Generic cancellation API, i.e. `Cancellation.h` and unit-tests of the API,
2. ClangdServer using the cancellation API.
3. LSP-specific bits, i.e. `cancelRequest` etc.

Not sure if it's worth splitting the last two, but splitting out the 
Cancellation API in a separate change could make review simpler.
But also happy to review the whole thing and land after it's done, feel free to 
pick whatever suits you best.




Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key>> CancellationTokenKey;
+} // namespace

kadircet wrote:
> ilya-biryukov wrote:
> > Having a `shared_ptr` key in the Context can cause data races (e.g. if we 
> > copy it concurrently from multiple threads).
> > I suggest we make `CancellationToken` move-only (i.e. disallow copies) and 
> > return `const CancellationToken&` when getting it from the context.
> As talked offline, copying std::shared_ptr is thread safe.
My mistake, thanks for explaining this one to me.
Copying of `CancellationToken`s LG!



Comment at: clangd/Cancellation.h:86
+/// Enables async tasks to check for cancellation signal, contains a read only
+/// token cached from context. Can be created with clangd::isCancelled. Totally
+/// thread-safe multiple threads can check for cancellation on the same token.

NIT: Maybe replace 'Totally thread-safe' with just `Thread-safe`? The latter 
conveys the same information.



Comment at: clangd/Cancellation.h:90
+public:
+  bool isCancelled() const { return Token ? static_cast(*Token) : false; 
}
+  operator bool() const { return isCancelled(); }

NIT: maybe use `Token->load()` instead of `static_cast(Token)`? Arguably, 
looks more concise.



Comment at: clangd/Cancellation.h:92
+  operator bool() const { return isCancelled(); }
+  friend CancellationToken isCancelled();
+

It's a bit confusing that this name clashes with a member function.
We seem to optimize for making this work anywhere:
```
  if (isCancelled())  // <-- get token from ctx, then use implicit conversion 
to check if it was cancelled
return llvm::makeError();
```

Which is probably a nice pattern. But we could have two functions that do the 
same thing with less magic (i.e. implicit conversions) involved at the call 
sites:
```
CancellationToken getCurrentCancellation();
void setCurrentCancellation(CancellationToken CT);

inline bool isCancelled() { return getCurrentCancellation().isCancelled(); }
```
Would allow to get rid of implicit conversion and friend function with the same 
signature as member. Would also make cases where we actually want to stash the 
token somewhere cleaner, e.g. instead of `CancellationToken CT = 
isCancelled()`, we'll have `CancellationToken CT = getCurrentCancellation()`.
WDYT?



Comment at: clangd/Cancellation.h:101
+
+/// Enables signalling a cancellation on an asyn task. Totally thread-safe, can
+/// trigger cancellation from multiple threads or make copies. Since contains a

s/asyn/async



Comment at: clangd/Cancellation.h:104
+/// std::shared_ptr inside copying is costy, avoid it as much as possible.
+class TaskHandle {
+public:

As discussed offline, maybe we should make `TaskHandle` move-only?
Conceptually, `TaskHandle` is an abstraction that represent some spawned async 
task and in that sense somewhat similar to futures.




Comment at: clangd/Cancellation.h:108
+  static TaskHandle create();
+  friend Context setCurrentCancellationToken(TaskHandle TH);
+

I wonder if we should have a `createCancellationToken()` method and a method to 
stash it in the context instead?
Would help if we want to make `TaskHandle` move-only and in general seems like 
a simpler user API, because things you put and get back out of the context is 
the same, i.e. it's a `CancellationToken`.
WDYT?



Comment at: clangd/ClangdLSPServer.cpp:74
 
+const std::string NormalizeRequestID(const json::Value &ID) {
+  std::string NormalizedID;

NIT: return `std::string`, const does not buys us anything here.



Comment at: clangd/ClangdLSPServer.cpp:354
+if (!List) {
+  return replyError(List.takeError());
+}

NIT: remove braces around short, single-statement branches



Comment at: clangd/ClangdLSPServer.cpp:631
+return;
+  const std::string &NormalizedID = NormalizeRequestID(*ID);
+  {

Use a value type here, i.e. `std::string` instead of `const std::string&`.



Comment at: clangd/ClangdLSPServer.cpp:642
+return;
+  const std::string &NormalizedID 

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50627#1198595, @hokein wrote:

> I think this patch is in a good scope (for empty preamble). I'd leave it as 
> it is. The failure of GoTodefinition test is caused by an inconsistent 
> behavior of using `lastBuiltPreamble`/`NewPreamble` in TUScheduler, I will 
> send out a separate patch fixing it (based on our discussion).


LG, thanks!

+ a few more nits to the tests




Comment at: unittests/clangd/TUSchedulerTests.cpp:333
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(Preamble->Preamble->Preamble.getBounds().Size,

NIT: just use `cantFail(std::move(Preamble))` to get the underlying result, no 
need for an extra `ASSERT_TRUE` here.



Comment at: unittests/clangd/TUSchedulerTests.cpp:348
+  // We expect to get an empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_EQ(Preamble->Preamble->Preamble.getBounds().Size,

NIT: also use `cantFail`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The last set of comments is for the previous version, might be missing some 
updates, sorry about that. Will make sure to review the one too!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Log on index request, remove FIXME that was addressed
- Remove SymbolID::forDecl, use existing helper instead


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
   EXPECT_TRUE(Results.Completions.empty());
 }
 
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+  std::unique_ptr Index;
+  if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.StaticIndex = Index.get();
+
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
   return false;
   return true;
 }
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
 
 Matcher Sig(std::string Label,
   std::vector Params) {
@@ -1590,6 +1599,51 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SignatureHelpTest, IndexDocumentation) {
+  Symbol::Details DocDetails;
+  DocDetails.Documentation = "Doc from the index";
+
+  Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+  Foo0.Detail = &DocDetails;
+  Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+  Foo1.Detail = &DocDetails;
+  Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0, Foo1, Foo2})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+  AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -171,12 +171,11 @@
 CodeCompleteOptions Opts);
 
 /// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand &Command,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand &Command,
+  PrecompiledPreamble const *Preamble, StringRef Contents,
+  Position Pos, IntrusiveRefCntPtr VFS,
+  std::shared_ptr PCHs, SymbolIndex *Index);
 
 // For index-based completion, we only consider:
 //   * symbols in namespaces or translation unit scopes (e.g. no class
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -687,18 +687,23 @@
   llvm::unique_function ResultsCallback;
 };
 
-using ScoredSignature =
-std::pair;
+struct ScoredSignature {
+  // When set, requires documentation to be requested from the index with this
+  // ID.
+  llvm::Optional IDForDoc;
+  SignatureInformation Signature;
+  SignatureQualitySignals Quality;
+};
 
 class SignatureHelpCollector final : public CodeCompleteConsumer {
-
 public:
   SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
- SignatureHelp &SigHelp)
-  : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
+ SymbolIndex *Index, SignatureHelp &SigHelp)
+  : CodeComplete

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+  LookupRequest IndexRequest;

hokein wrote:
> nit: do we want to log anything here? It may be useful for debug.
Definitely useful. Thanks!



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

hokein wrote:
> We already have this similar function in clangd/AST.h.
Thanks for pointing this out.
It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern 
would probably be a somewhat broken layering, right? E.g. `Index.h` should not 
directly depend on anything AST-specific


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> drive by: I think this should  be `vlog` or `dlog`.
Code completion also logs the number of results from sema, index, etc. using 
the `log()` call.
The added log message looks similar, so trying to be consistent with the rest 
of the code in this file.

Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave it 
to a separate patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > drive by: I think this should  be `vlog` or `dlog`.
> > > > Code completion also logs the number of results from sema, index, etc. 
> > > > using the `log()` call.
> > > > The added log message looks similar, so trying to be consistent with 
> > > > the rest of the code in this file.
> > > > 
> > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > > > leave it to a separate patch.
> > > I'm not sure which level code completion log messages should be in, but I 
> > > think we should follow the guidelines in the logger documentation instead 
> > > of the existing uses.
> > > 
> > > Quote from Logger.h
> > > ```
> > > // log() is used for information important to understanding a clangd 
> > > session.
> > > // e.g. the names of LSP messages sent are logged at this level.
> > > // This level could be enabled in production builds to allow later 
> > > inspection.
> > > 
> > > // vlog() is used for details often needed for debugging clangd sessions.
> > > // This level would typically be enabled for clangd developers.
> > > ```
> > My recent experience of debugging some weird GotoDef issues tells me that 
> > log of index should be on production (since it is a non-trivial part in a 
> > clangd session), it would be really helpful to understand what is going on. 
> I agree that they are useful for debugging, but they might not be interesting 
> to end-users. And I think that's why there is `vlog`. Clangd developers could 
> use a different log level with `--log` flag.
I agree that `vlog` is probably a better fit here, but still think we should 
change it across `CodeComplete.cpp` in a single commit, rather than partially 
apply the guidelines to different parts of the file.

However, if everyone agrees we should use `vlog` here, happy to use `vlog` too 
and also send a patch to update all the rest of the usages.
The situation I'm trying to avoid is:
1. We commit `vlog` here.
2. Someone argues that using `log` is actually better and we decide to not 
change other usages to `log`.
3. We end up with inconsistent choices across this file: `vlog` for signature 
help and `log` for code completion.

But if there's an agreement that we should use `vlog` everywhere, more than 
happy to change it :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 161011.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Expose getDeclComment instead of getDocComment


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726

Files:
  clangd/CodeComplete.cpp
  clangd/CodeCompletionStrings.cpp
  clangd/CodeCompletionStrings.h


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,8 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will be returned. It is used to workaround crashes when parsing
-/// comments in the stale headers, coming from completion preamble.
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders);
+/// Similar to getDocComment, but returns the comment for a NamedDecl.
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &D);
 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector::at(size_type) const, this returns:
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -52,34 +52,20 @@
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";
   auto *Decl = Result.getDeclaration();
-  if (!Decl || llvm::isa(Decl)) {
+  if (!Decl)
+return "";
+  return getDeclComment(Ctx, *Decl);
+}
+
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
+  if (llvm::isa(Decl)) {
 // Namespaces often have too many redecls for any particular redecl comment
 // to be useful. Moreover, we often confuse file headers or generated
 // comments with namespace comments. Therefore we choose to just ignore
 // the comments for namespaces.
 return "";
   }
-  const RawComment *RC = getCompletionComment(Ctx, Decl);
-  if (!RC)
-return "";
-
-  // Sanity check that the comment does not come from the PCH. We choose to not
-  // write them into PCH, because they are racy and slow to load.
-  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
-  std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), 
Ctx.getDiagnostics());
-  if (!looksLikeDocComment(Doc))
-return "";
-  return Doc;
-}
-
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders) {
-  auto *Func = Result.getFunction();
-  if (!Func)
-return "";
-  const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
+  const RawComment *RC = getCompletionComment(Ctx, &Decl);
   if (!RC)
 return "";
   // Sanity check that the comment does not come from the PCH. We choose to not
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -729,8 +729,9 @@
   // FIXME: for headers, we need to get a comment from the index.
   ScoredSignatures.push_back(processOverloadCandidate(
   Candidate, *CCS,
-  getParameterDocComment(S.getASTContext(), Candidate, CurrentArg,
- /*CommentsFromHeaders=*/false)));
+  Candidate.getFunction()
+  ? getDeclComment(S.getASTContext(), *Candidate.getFunction())
+  : ""));
 }
 std::sort(ScoredSignatures.begin(), ScoredSignatures.end(),
   [](const ScoredSignature &L, const ScoredSignature &R) {


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,8 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50385#1193600, @ioeric wrote:

> In https://reviews.llvm.org/D50385#1193545, @hokein wrote:
>
> > In https://reviews.llvm.org/D50385#1191914, @ioeric wrote:
> >
> > > 2 high-level questions:
> > >
> > > 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could 
> > > store occurrences as extra payload of `Symbol`?
> >
> >
> > Storing occurrences in `Symbol` structure is easy to misuse by users IMO -- 
> > if we go through this way, we will end up having a `getOccurrences`-like 
> > method in `Symbol` structure. Once users get the `Symbol` instance, it is 
> > natural for them to call `getOccurrences` to get all occurrences of the 
> > symbol. However this `getOccurrences` method doesn't do what users expected 
> > (just returning an incomplete set of results or empty). To query the symbol 
> > occurrences, we should always use index interface.
> >
> > Therefore, I think we should try to avoid these confusions in the design.
>
>
> Hmm, I think this is the same for other symbol payload e.g. definition can be 
> missing for a symbol. And it seems to me that the concern is on the 
> SymbolSlab level: if a slab is for a single TU, users should expect missing 
> information; if a slab is merged from all TUs, then users can expect 
> "complete" information. I think it's reasonable to assume that users of 
> SymbolSlab are aware of this. I think it's probably not worth the overhead of 
> maintaining and using two separate slabs.


I think it's reasonable to keep occurrences away from Symbol's Detail field. 
Stashing them together is only fine for the collector API, having any way to 
directly access occurrences through `Symbol` will be totally confusing for all 
the other users.
E.g., the `Index::lookup()` will not provide occurrences in the `Symbol` 
instances it returns, and if the accessors for those will be there it will only 
add confusion. So +1 to keeping them out of the `Symbol` class.

On the other hand, `SymbolSlab` feels like a perfectly reasonable place to 
store the occurrences in addition to the symbols themselves and it feels we 
should reuse its memory arena for storing any strings we need to allocate, etc.

>>> 2. Could we merge `SymbolOccurrenceCollector` into the existing 
>>> `SymbolCollector`? They look a lot alike. Having another index data 
>>> consumer seems like more overhead on the user side.
>> 
>> The `SymbolOccurrenceCollector` has many responsibilities (collecting 
>> declaration, definition, code completion information etc), and the code is 
>> growing complex now. Merging the `SymbolOccurrenceCollector` to it will make 
>> it more
> 
> Although the existing `SymbolCollector` supports different options, I think 
> it still has a pretty well-defined responsibility: gather information about 
> symbols. IMO, cross-reference is one of the property of symbol, and I don't 
> see strong reasons to keep them separated.
> 
>> complicated -- we will introduce more option flags like 
>> `collect-symbol-only`, `collect-occurrence-only` to configure it for our 
>> different use cases (we need to the implementation detail clearly in order 
>> to make a correct option for `SymbolCollector`).
> 
> I think these options are reasonable if they turn out to be necessary. And 
> making the SymbolCollector more complicated doesn't seem to be a problem if 
> we are indeed doing more complicated work, but I don't think this would turn 
> into a big problem as logic of xrefs seems pretty isolated.  Conversely, I 
> think implementing xrefs in a separate class would likely to cause more 
> duplicate and maintenance, e.g. two sets of options, two sets of 
> initializations or life-time tracking of collectors (they look a lot alike), 
> the same boilerplate factory code in tests, passing around two collectors in 
> user code.
> 
>> And I can foresee these two collectors might be run at different point 
>> (runWithPreamble vs runWithAST) in dynamic index.
> 
> With some options, this should be a problem I think?

+1 to merging into the `SymbolCollector`. Keeping the responsibilities separate 
inside a single class should be easy, e.g. something like that should be simple 
enough:

  SymbolCollector::handleDeclOccurence(args) {
this->processForSymbol(args); // handles keeping the Symbol structure 
up-to-date, i.e. adds definition locations, etc.
this->processForOccurrences(args); // appends occurrences to a list of 
xrefs.
  };

The main advantage that we get is less clang-specific boilerplate. The less 
`IndexDataConsumer`s, `FrontendActionFactory`s, `FrontendAction`s we create, 
the more focused and concise our code is.
And in that case, `SymbolCollector` is already handling those responsibilities 
for us and reusing looks like a good idea.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llv

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Just drive-by comments, the maintainers of the code are in a much better 
position to give feedback, of course.
Nevertheless, my few cents:

- Getting rid of a regex in favor of the explicit loop is definitely a good 
thing. It's incredibly how much time is spent there when serializing big chunks 
of YAML (our case in clangd).
- Other changes are definitely less of a win performance-wise and I personally 
find the new code a bit harder to read than before, so don't feel as confident 
about those.
- Actual correctness fixes are a good thing, but could go in as a separate 
patch.

I would suggest splitting the patch into two: (1) rewriting the regex, (2) 
other changes. (1) is such a clear win, that it would be a pitty to have it 
delayed by reviewing other parts of the patch :-)




Comment at: llvm/include/llvm/Support/YAMLTraits.h:466
+  static const char HexChars[] = "0123456789abcdefABCDEF";
+  static const char OctalChars[] = "01234567";
+  bool ParseHex = S.startswith("0x");

I would argue the previous version of parsing hex and octal chars was more 
readable.
And I'm not sure the new heavily optimized version is more performant too.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:494
+  bool FoundExponent = false;
+  for (size_t I = 0; I < Tail.size(); ++I) {
+char Symbol = Tail[I];

A more structured way to parse a floating numbers would be to:

1. skip digits until we find `.` or exponent (`e`)
2. if we find `.`, skip digits until we find an exponent.
3. if we find an exponent, skip the next symbol if it's `'+'` or `'-'`, then 
skip digits until the end of the string.

having a code structure that mirrors that would make the code more readable.



https://reviews.llvm.org/D50839



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

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

Will be used for updating the dynamic index on updates to the open files.
Currently we collect only information coming from the preamble
AST. This has a bunch of limitations:

- Dynamic index misses important information from the body of the file, e.g. 
locations of definitions.
- XRefs cannot be collected at all, since we can only obtain full information 
for the current file (preamble is parsed with skipped function bodies, 
therefore not reliable).

This patch only adds the new callback, actually updating the index
will be added in a follow-up patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847

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

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -17,10 +17,11 @@
 
 namespace clang {
 namespace clangd {
+namespace {
 
 using ::testing::_;
-using ::testing::Each;
 using ::testing::AnyOf;
+using ::testing::Each;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -30,6 +31,18 @@
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
 
+class NoopParsingCallbacks : public ParsingCallbacks {
+public:
+  static NoopParsingCallbacks& instance() {
+static NoopParsingCallbacks* Instance = new NoopParsingCallbacks;
+return *Instance;
+  }
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {}
+  void onMainAST(PathRef Path, ParsedAST &AST) override {}
+};
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -45,7 +58,7 @@
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -102,7 +115,7 @@
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+NoopParsingCallbacks::instance(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,11 +142,10 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::seconds(1),
-  ASTRetentionPolicy());
+TUScheduler S(
+getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true, NoopParsingCallbacks::instance(),
+/*UpdateDebounce=*/std::chrono::seconds(1), ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
 auto Path = testPath("foo.cpp");
 S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
@@ -163,7 +175,7 @@
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -261,7 +273,7 @@
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -313,7 +325,7 @@
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -346,7 +358,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+  /*StorePreambleInMemory=*/true, NoopParsingCallbacks::instance(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -399,7 +411,7 @@
 TEST_F(TUSchedulerTests, NoCh

[PATCH] D50695: [clangd] Always use the latest preamble

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695



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

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, but note the comment: we don't need `ASSERT_TRUE(bool(Preamble))`.




Comment at: unittests/clangd/TUSchedulerTests.cpp:333
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(cantFail(std::move(Preamble))

Remove `ASSERT_TRUE(bool(Preamble))`?
We assume everyone runs the tests run with assertions enable on every commit 
(and builtbots will catch the ones that don't), so `cantFail` will handle the 
error case for us.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Turn ifs to ternary ops


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726

Files:
  clangd/CodeComplete.cpp
  clangd/CodeCompletionStrings.cpp
  clangd/CodeCompletionStrings.h


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,8 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will be returned. It is used to workaround crashes when parsing
-/// comments in the stale headers, coming from completion preamble.
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders);
+/// Similar to getDocComment, but returns the comment for a NamedDecl.
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &D);
 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector::at(size_type) const, this returns:
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -51,44 +51,26 @@
   // get this declaration, so we don't show documentation in that case.
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";
-  auto *Decl = Result.getDeclaration();
-  if (!Decl || llvm::isa(Decl)) {
+  return Result.getDeclaration() ? getDeclComment(Ctx, 
*Result.getDeclaration())
+ : "";
+}
+
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
+  if (llvm::isa(Decl)) {
 // Namespaces often have too many redecls for any particular redecl comment
 // to be useful. Moreover, we often confuse file headers or generated
 // comments with namespace comments. Therefore we choose to just ignore
 // the comments for namespaces.
 return "";
   }
-  const RawComment *RC = getCompletionComment(Ctx, Decl);
-  if (!RC)
-return "";
-
-  // Sanity check that the comment does not come from the PCH. We choose to not
-  // write them into PCH, because they are racy and slow to load.
-  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
-  std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), 
Ctx.getDiagnostics());
-  if (!looksLikeDocComment(Doc))
-return "";
-  return Doc;
-}
-
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders) {
-  auto *Func = Result.getFunction();
-  if (!Func)
-return "";
-  const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
+  const RawComment *RC = getCompletionComment(Ctx, &Decl);
   if (!RC)
 return "";
   // Sanity check that the comment does not come from the PCH. We choose to not
   // write them into PCH, because they are racy and slow to load.
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), 
Ctx.getDiagnostics());
-  if (!looksLikeDocComment(Doc))
-return "";
-  return Doc;
+  return looksLikeDocComment(Doc) ? Doc : "";
 }
 
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -729,8 +729,9 @@
   // FIXME: for headers, we need to get a comment from the index.
   ScoredSignatures.push_back(processOverloadCandidate(
   Candidate, *CCS,
-  getParameterDocComment(S.getASTContext(), Candidate, CurrentArg,
- /*CommentsFromHeaders=*/false)));
+  Candidate.getFunction()
+  ? getDeclComment(S.getASTContext(), *Candidate.getFunction())
+  : ""));
 }
 std::sort(ScoredSignatures.begin(), ScoredSignatures.end(),
   [](const ScoredSignature &L, const ScoredSignature &R) {


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeCompletionStrings.cpp:52
   // get this declaration, so we don't show documentation in that case.
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";

ioeric wrote:
> `RK_Pattern` can also have declaration. Do we want to consider those?
See the FIXME at the top, we don't have the API exposed from clang to get the 
proper declarations (IIRC, this only shows up for ObjC properties and we need 
to look at both getters and setters when looking for a comment, and the clang 
code does not expose the API to do that).

We should probably fix this, but that's a separate issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50847#1202658, @ioeric wrote:

> > Dynamic index misses important information from the body of the file, e.g. 
> > locations of definitions
> >  XRefs cannot be collected at all, since we can only obtain full 
> > information for the current file (preamble is parsed with skipped function 
> > bodies, therefore not reliable).
>
> These seem to be known limitations of the dynamic index. The dynamic index, 
> in general, only handles things that are in headers; information in the main 
> file can usually be obtained from Sema/AST (outside of the index). For 
> example, definition/xrefs locations can be merged from index and the AST.


We still want this information in the index. One can obtain this information 
for the **current** file. However, we still need to keep this info in the index 
for definitions in the other open files.

>> This patch only adds the new callback, actually updating the index will be 
>> added in a follow-up patch.
> 
> How do we plan to merge symbols from the preamble callback and the main file 
> callback? The current FileIndex only allows swapping all symbols.

There are multiple ways to do that, lowest overhead is having two separate 
slabs: one for preamble symbols, another for symbols in the main file. They 
will be merged on-the-go when requested.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric.
Herald added subscribers: arphaman, jkorous, MaskRay.

It was previously only indexing the preamble decls. The new
implementation will index both the preamble and the main AST and
report both sets of symbols, preferring the ones from the main AST
whenever the symbol is present in both.
The symbols in the main AST slab always store all information
available in the preamble symbols, possibly adding more,
e.g. definition locations.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889

Files:
  clangd/ClangdServer.cpp
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.h
  clangd/index/SymbolCollector.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TestTU.cpp

Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
 
 SymbolSlab TestTU::headerSymbols() const {
   auto AST = build();
-  return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+  return indexPreambleAST(AST.getASTContext(), AST.getPreprocessorPtr());
 }
 
 std::unique_ptr TestTU::index() const {
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -51,28 +51,28 @@
   FileSymbols FS;
   EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
 
-  FS.update("f1", numSlab(1, 3));
+  FS.updatePreamble("f1", numSlab(1, 3));
   EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
   UnorderedElementsAre("1", "2", "3"));
 }
 
 TEST(FileSymbolsTest, Overlap) {
   FileSymbols FS;
-  FS.update("f1", numSlab(1, 3));
-  FS.update("f2", numSlab(3, 5));
+  FS.updatePreamble("f1", numSlab(1, 3));
+  FS.updatePreamble("f2", numSlab(3, 5));
   EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
   UnorderedElementsAre("1", "2", "3", "3", "4", "5"));
 }
 
 TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
   FileSymbols FS;
 
-  FS.update("f1", numSlab(1, 3));
+  FS.updatePreamble("f1", numSlab(1, 3));
 
   auto Symbols = FS.allSymbols();
   EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
 
-  FS.update("f1", nullptr);
+  FS.updatePreamble("f1", nullptr);
   EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
   EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
 }
@@ -93,7 +93,7 @@
   File.HeaderFilename = (Basename + ".h").str();
   File.HeaderCode = Code;
   auto AST = File.build();
-  M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr());
+  M.updatePreamble(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr());
 }
 
 TEST(FileIndexTest, CustomizedURIScheme) {
@@ -149,13 +149,13 @@
   Req.Scopes = {"ns::"};
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X"));
 
-  M.update("f1.cpp", nullptr, nullptr);
+  M.updatePreamble("f1.cpp", nullptr, nullptr);
   EXPECT_THAT(match(M, Req), UnorderedElementsAre());
 }
 
 TEST(FileIndexTest, RemoveNonExisting) {
   FileIndex M;
-  M.update("no.cpp", nullptr, nullptr);
+  M.updatePreamble("no.cpp", nullptr, nullptr);
   EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre());
 }
 
@@ -255,7 +255,7 @@
   std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, &Ctx, std::move(PP));
+Index.updatePreamble(FilePath, &Ctx, std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -85,6 +85,7 @@
 SourceLocation Loc) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
+  SymbolSlab::Builder takeBuilder() { return std::move(Symbols); }
 
   void finish() override;
 
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -267,6 +267,8 @@
   return I == SymbolIndex.end() ? nullptr : &Symbols[I->second];
 }
 
+llvm::ArrayRef symbols() const { return Symbols; }
+
 // Consumes the builder to finalize the slab.
 SymbolSlab build() &&;
 
Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -41,16 +41,23 @@
 public:
   /// \brief Updates all symbols in a file. If \p Slab is nullptr, symbols for
   /// \p Path will be removed.
-  void update(PathRef Path, std::unique_ptr Slab);
+  void updatePreamble(PathRef Path, std::unique_ptr Slab);
+  void updateMainFile(PathRef Path, SymbolSlab::Builder Bu

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@ioeric, specifically, see https://reviews.llvm.org/D50889 for an updated 
FileIndex


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This is still WIP, but wanted to share anyway the progress anyway.
Need to fix the FIXMEs currently added to the code and add tests.




Comment at: clangd/index/FileIndex.cpp:81
   if (!Slab)
 FileToSlabs.erase(Path);
   else

We need to be more careful here now that we have to slabs in the list. Should 
only erase when both are removed.



Comment at: clangd/index/FileIndex.cpp:87
+void FileSymbols::updateMainFile(PathRef Path, SymbolSlab::Builder Builder) {
+  // FIXME(ibiryukov): most of this work should be done outside the lock.
+  std::lock_guard Lock(Mutex);

This should definitely be done outside the lock, only the actual swap of the 
symbol tables should be there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov planned changes to this revision.
ilya-biryukov added a comment.

As discussed offline, we could instead have a two-layer scheme for dynamic 
index. A top layer will contain main file symbols, second layer will contain 
preamble symbols.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as

kadircet wrote:
> ioeric wrote:
> > IIRC, the goal of this patch is to allow disabling snippet templates for 
> > function parameters while still allow appending `()` or `($0)` to function 
> > candidates. If that's still the case, I think what we want is probably an 
> > option like `DisableSnippetTemplateForFunctionArgs`? 
> Yeah, that makes totally more sense. Thanks!
- Maybe use positive option names to avoid double negation?
- Maybe also use a shorter name? E.g. removing 'template' and 'disable' from 
the name gives a name that does not look any worse: `FunctionArgSnippets`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Why do we want an alternative mode for transferring fix-its and notes? Any 
reason why our current model is bad?
One thing that comes to mind is inconsistency between clang and our model, but 
I wonder where would it affect the user experience in a bad way?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs, but please take a look at the `CancelParams` handling problem. I 
believe there might be a potential bug hiding there :-)




Comment at: clangd/Cancellation.h:87
+/// Enables async tasks to check for cancellation signal, contains a read only
+/// token cached from context. Can be created with clangd::isCancelled. It is
+/// thread-safe, multiple threads can check for cancellation on the same token.

s/Can be created with `clangd::isCancelled()`/Tokens for the currently running 
task can be obtained via clangd::getCurrentCancellationToken/?
(to focus on which kind of tokens can be obtained from the tokens)

Another idea: maybe add a reference no why someone would want to use 
`CancellationToken` instead of `isCancelled()` helper, e.g.
```
/// If cancellation checks are rare, one could use the isCancelled() helper to 
simplify the code.
/// However, if cancellation checks are frequent, the guideline is first obtain 
the CancellationToken for the currently running task with 
getCurrentCancellationToken() and do cancel checks using it to avoid extra 
lookups in the Context
```



Comment at: clangd/Cancellation.h:137
+  void log(llvm::raw_ostream &OS) const override {
+OS << "Task got cancelled.";
+  }

s/got/was?



Comment at: clangd/ClangdLSPServer.cpp:621
+  std::lock_guard Lock(TaskHandlesMutex);
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it != TaskHandles.end()) {

Wouldn't it work incorrectly for string IDs?
When normalizing `json::Value` from `getRequestID`, we simply print out json. 
For strings, this should result in quoted output, e.g. `"req"`. However, when 
parsing `CancelParams`, we parse this json. So we'll end up inserting with a 
key `"req"` and erasing with a key 'req' (without the quotes).



Comment at: clangd/ClangdLSPServer.cpp:643
+  std::lock_guard Lock(TaskHandlesMutex);
+  TaskHandles.insert({NormalizedID, std::move(TH)});
+}

Could we `elog` if `insert` indicated that a value already exists in a map? 
This would probably mean malformed user input, but could also be immensely 
helpful for debugging.



Comment at: clangd/ClangdLSPServer.h:179
+  // associated with only their thread.
+  void CleanupTaskHandle();
+  void StoreTaskHandle(TaskHandle TH);

kadircet wrote:
> ilya-biryukov wrote:
> > These two methods look like a nice suit for better wrapped in a 
> > RAII-object. It would add the value to the associated map on construction 
> > and remove on destruction.
> > One problem is that all operations calls would become weird, because we'll 
> > have to move this RAII object around and moving into lambdas is not fun 
> > without C++14 (which allows `[x=std::move(y)]() {}`..
> > 
> > Maybe add a comment that calls to this function should always be paired and 
> > a FIXME that we should have an API that enforces this?
> > 
> Yeah that would be really nice to have them in a RAII object, but the thing 
> is cleanup can occur only after the task dies. Which we don't directly know 
> since threads are being detached. So the only way to trigger destruction is 
> at the callback, which is where I call cleanup currently. Maybe we can store 
> the object within context to trigger destructor at the end of the thread, but 
> still don't think it would look any better or ease the job of the caller.
Having these paired method calls is probably fine for the first iteration, but 
we should abstract away this pattern when supporting cancellations for more 
operations.
LG with a FIXME.



Comment at: clangd/ClangdServer.cpp:152
 
+  auto CancellableTaskHandle = TaskHandle::create();
+  auto CT = CancellableTaskHandle.createCancellationToken();

Maybe use a shorter name? E.g. `Task` or `TH` :-)



Comment at: clangd/ClangdServer.cpp:153
+  auto CancellableTaskHandle = TaskHandle::create();
+  auto CT = CancellableTaskHandle.createCancellationToken();
   // Copy PCHs to avoid accessing this->PCHs concurrently

Instead of moving cancellation token around explicitly, we could stash in a 
context right away and it would nicely propagate into the thread. This looks 
like less boilerplate.
E.g.
```
  auto CancellableTaskHandle = TaskHandle::create();
  WithContext 
ContextWithCancellation(setCurrentCancellationToken(TaskHandle.createCancellationToken()));

  auto Task = [](File, Callback CB, 
llvm::Expected IP) {
if (getCancellationToken().isCancelled())
  return CB(make_error());
   };
   /// rest of the code is the same
```



Comment at: clangd/ClangdServer.cpp:178
 
-  WorkScheduler.runWithPreamble("CodeComplete", File,
-Bind(Task, File.str(), std::move(CB)));
+  WorkScheduler.runWithPreamble(
+  "CodeComplete", File,

Thinking out loud: the TaskH

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



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

Are we going to support the `IncludeDeclaration` option?
What should its semantics be?

I'm trying to figure out a better name for it, can't get what should it do by 
looking at the code at the moment :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Reverted changes to FileIndex, use merged index instead


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h

Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -64,7 +64,11 @@
   /// nullptr, this removes all symbols in the file.
   /// If \p AST is not null, \p PP cannot be null and it should be the
   /// preprocessor that was used to build \p AST.
-  void update(PathRef Path, ASTContext *AST, std::shared_ptr PP);
+  /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all
+  /// top level decls obtained from \p AST are indexed.
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);
 
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
@@ -86,8 +90,12 @@
 /// Retrieves namespace and class level symbols in \p AST.
 /// Exposed to assist in unit tests.
 /// If URISchemes is empty, the default schemes in SymbolCollector will be used.
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
-llvm::ArrayRef URISchemes = {});
+/// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all top
+/// level decls obtained from \p AST are indexed.
+SymbolSlab
+indexAST(ASTContext &AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None,
+ llvm::ArrayRef URISchemes = {});
 
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -8,15 +8,16 @@
 //===--===//
 
 #include "FileIndex.h"
-#include "SymbolCollector.h"
 #include "../Logger.h"
+#include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 namespace clangd {
 
 SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
+llvm::Optional> TopLevelDecls,
 llvm::ArrayRef URISchemes) {
   SymbolCollector::Options CollectorOpts;
   // FIXME(ioeric): we might also want to collect include headers. We would need
@@ -38,10 +39,13 @@
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
 
-  std::vector TopLevelDecls(
-  AST.getTranslationUnitDecl()->decls().begin(),
-  AST.getTranslationUnitDecl()->decls().end());
-  index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+  std::vector Storage;
+  if (!TopLevelDecls) {
+Storage.assign(AST.getTranslationUnitDecl()->decls().begin(),
+   AST.getTranslationUnitDecl()->decls().end());
+TopLevelDecls = Storage;
+  }
+  index::indexTopLevelDecls(AST, *TopLevelDecls, Collector, IndexOpts);
 
   return Collector.takeSymbols();
 }
@@ -81,13 +85,14 @@
 }
 
 void FileIndex::update(PathRef Path, ASTContext *AST,
-   std::shared_ptr PP) {
+   std::shared_ptr PP,
+   llvm::Optional> TopLevelDecls) {
   if (!AST) {
 FSymbols.update(Path, nullptr);
   } else {
 assert(PP);
 auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, URISchemes);
+*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
 FSymbols.update(Path, std::move(Slab));
   }
   auto Symbols = FSymbols.allSymbols();
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -207,6 +207,27 @@
 
   tooling::CompileCommand getCompileCommand(PathRef File);
 
+  /// Manages dynamic index for open files. Each file might contribute two sets
+  /// of symbols to the dynamic index: symbols from the preamble and symbols
+  /// from the file itself. Those have different lifetimes and we merge results from both 
+  class DynamicIndex : public ParsingCallbacks {
+  public:
+DynamicIndex(std::vector URISchemes);
+
+SymbolIndex &index() const;
+
+void onPreambleAST(PathRef Path, ASTContext &Ctx,
+  std::shared_ptr PP) override;
+void onMainAST(PathRef Path, ParsedAST &AST) override;
+
+  private:
+FileIndex PreambleIdx;
+FileIndex MainFileIdx;
+/// Merged view into both indexes. Merges are performed in a similar manner
+/// to the merges of dynamic and static index.
+std::unique_ptr MergedIndex;
+  };
+
   GlobalCompilationDatabase &CDB;
   DiagnosticsConsumer &DiagConsumer;
   FileSystemProvider &FSProvider;
@@ -221,10 +242,8 @@
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Still missing tests, but otherwise should be good to review.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/TUScheduler.cpp:406
 // We only need to build the AST if diagnostics were requested.
 if (WantDiags == WantDiagnostics::No)
   return;

hokein wrote:
> The AST might not get built if `WantDiags::No`, and will be built in 
> `runWithAST`.
I suggest we keep it a known limitation and not rebuild the index in that case.
The original intent of `WantDiags::No` was to update the contents of the file, 
so that upcoming request can see the new contents (e.g. this is a hack to avoid 
passing overriden contents of the file in the request itself).
`WantDiags::No` is always followed by an actual request that wants the 
diagnostics (and will, therefore, build the AST and emit the callback).

Alternatively, we could unify the code paths building the AST. Would be a bit 
more intrusive change, but I guess it's worth it.



Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};

hokein wrote:
> Does the callback get called every time we built an AST? clangd only has 3 
> idle ASTs, if the AST is not there, we rebuild it when needed (even the 
> source code is not changed), and we will update the dynamic index, which 
> seems unnecessary.
> 
> It may rarely happen, 3 idle ASTs  might cover most cases, I think? 
Currently we only call this when AST is built for diagnostics, so we will have 
only a single callback, even if the AST will be rebuilt multiple times because 
it was flushed out of the cash (as long as the file contents didn't change, of 
course).

So we do behave optimally in that case and I suggest we keep it this way


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This change LG, but I would not commit it before we have an actual 
implementation.
As soon as we have the `references` function in `ClangdUnit.cpp` implemented, 
the merge of this change should be trivial.

Is there any value in committing empty stubs before an actual implementation is 
ready?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:80
+  if (NormalizedID.front() == '"')
+NormalizedID = NormalizedID.substr(1, NormalizedID.size() - 2);
+  return NormalizedID;

This still misses string escaping issues. E.g. `"` will get quoted as `\"`, etc.
I suggest we actually switch on this json value kind.
Better yet, reuse the part of `fromJSON(CancelParams)`  that does the 
request-id parsing.



Comment at: clangd/ClangdServer.cpp:166
 
+if (isCancelled())
+  return CB(llvm::make_error());

NIT: check for it even before looking at the preamble?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Having unimplemented stubs in the codebase creates some confusion, but that's 
my personal opinion. Not terribly opposed to that if others feel it's the right 
way to go.

For experiments, we could simply patch this in locally without committing 
upstream. With git it's incredibly easy.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:

ioeric wrote:
> As chatted offline, I have questions about the motivation of the 
> `CancellationToken` class. Intuitively, it seems to me that this can be 
> merged into `TaskHandle`, and we can simply stash the `TaskHandle` instead of 
> a token into the context. There would be fewer states to maintain, and the 
> design would be a bit more straightforward. I might be missing 
> context/concerns here, so I'd like to hear what you and Ilya think. 
> @ilya-biryukov 
I am for splitting cancellation checks from cancellation triggers.
The processing code only needs to check if it was cancelled and exposing the 
`cancel()` does not add any useful functionality, merely ways to misuse it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.
Herald added a subscriber: kadircet.

Just to make sure I fully understand the use-case: could you elaborate a little 
more? Do you need to get exactly the same set of notes that clang provides?
Our current model seems to follow the clang's model, but it removes some notes 
and shows them as fix-its for the main diagnostic instead.
(We think it should provide a better user experience, because those notes do 
look like a way to present a fix-it to the user when you have only text-based 
output and they seem redundant when one has an editor-based UI with fix-its).

Not opposed to changing the model to meet other needs, but want to make sure 
what we're currently doing in clangd makes sense and understand your use-case 
better.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814



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

[PATCH] D50703: [clangd] NFC: Mark Workspace Symbol feature complete in the documentation

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50703#1205167, @malaperle wrote:

> Nah, I guess other things are partial too, like Go to Definition. Let's 
> assume they will be completed "soon" ;)


https://reviews.llvm.org/D50889 should finally add the symbols from the main 
files. Sorry for the delays.


Repository:
  rL LLVM

https://reviews.llvm.org/D50703



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > As chatted offline, I have questions about the motivation of the 
> > > `CancellationToken` class. Intuitively, it seems to me that this can be 
> > > merged into `TaskHandle`, and we can simply stash the `TaskHandle` 
> > > instead of a token into the context. There would be fewer states to 
> > > maintain, and the design would be a bit more straightforward. I might be 
> > > missing context/concerns here, so I'd like to hear what you and Ilya 
> > > think. @ilya-biryukov 
> > I am for splitting cancellation checks from cancellation triggers.
> > The processing code only needs to check if it was cancelled and exposing 
> > the `cancel()` does not add any useful functionality, merely ways to misuse 
> > it.
> Couldn't we prevent this by passing only const handle references to users who 
> are not expect to call `cancel()`?
As long as `TaskHandle` does not have a copy constructor, that LG.
I.e. the scheme that seems to work is:
1. return `shared_ptr` from async computations like code complete.
2. stash `shared_ptr` into the context.
3. return `const TaskHandle*` from the context.

This should give a simpler implementation.
The cons from my point of view are:
1. We return copyable task handles (i.e. the shared_ptr) from the API by 
default. This provokes usages that don't take ownership issues into account, 
i.e. it's easy to just copy the task everywhere without having clear ownership 
of it. As long as it's just cancellation there, we're probably fine, but if we 
add more stuff into it that might become a problem.
2. We expose implementation details (i.e. shared_ptr) by removing the layer of 
abstraction. This makes potential refactorings of the client code harder.

In general, I think having our own wrappers there opens up more room for 
experiments with the API later (the API is very constrained, so it's easy to 
refactor the implementation).  OTOH, if we believe the design is right or 
changing the clients is not too much work (which is probably true), either way 
is fine with me.

But either solution has its pros and cons, I'm happy with trying out any of 
those on code completion, see how it goes and decide whether we want to change 
anything before finalizing our design and adding cancellation to all operations 
in clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



___
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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

1. Can we put the helper that executes the tasks asynchronously with a stack 
size into llvm's threading library (i.e. somewhere close to 
`llvm::execute_on_thread`?) All platform-specific code is already there, we can 
reuse most of the implementation too.
2. I wonder if we should expose the stack size as an option to clangd?




Comment at: clangd/Threading.cpp:57
+template 
+static void executeOnThreadAndDetach(UserFnPtr UserFn, Data UserData) {
+#if !defined(__APPLE__)

Maybe add a parameter for the stack size to this function? (And document it's 
only taken into account when pthread-based implementation is used)



Comment at: clangd/Threading.cpp:57
+template 
+static void executeOnThreadAndDetach(UserFnPtr UserFn, Data UserData) {
+#if !defined(__APPLE__)

ilya-biryukov wrote:
> Maybe add a parameter for the stack size to this function? (And document it's 
> only taken into account when pthread-based implementation is used)
Maybe accept a `llvm::unique_function` instead of `UserFn` and `Data`? 
This split is clearly implementation detail of pthreads-based solution. 
`std::thread` does not need it.


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] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Move DynamicIndex into ClangdServer.cpp
- Rename FileIdx to DynamicIdx
- Avoid modifying input args


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h

Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -64,7 +64,11 @@
   /// nullptr, this removes all symbols in the file.
   /// If \p AST is not null, \p PP cannot be null and it should be the
   /// preprocessor that was used to build \p AST.
-  void update(PathRef Path, ASTContext *AST, std::shared_ptr PP);
+  /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all
+  /// top level decls obtained from \p AST are indexed.
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);
 
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
@@ -86,8 +90,12 @@
 /// Retrieves namespace and class level symbols in \p AST.
 /// Exposed to assist in unit tests.
 /// If URISchemes is empty, the default schemes in SymbolCollector will be used.
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
-llvm::ArrayRef URISchemes = {});
+/// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all top
+/// level decls obtained from \p AST are indexed.
+SymbolSlab
+indexAST(ASTContext &AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None,
+ llvm::ArrayRef URISchemes = {});
 
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -8,15 +8,16 @@
 //===--===//
 
 #include "FileIndex.h"
-#include "SymbolCollector.h"
 #include "../Logger.h"
+#include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 namespace clangd {
 
 SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
+llvm::Optional> TopLevelDecls,
 llvm::ArrayRef URISchemes) {
   SymbolCollector::Options CollectorOpts;
   // FIXME(ioeric): we might also want to collect include headers. We would need
@@ -38,10 +39,14 @@
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
 
-  std::vector TopLevelDecls(
-  AST.getTranslationUnitDecl()->decls().begin(),
-  AST.getTranslationUnitDecl()->decls().end());
-  index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+  std::vector DeclsToIndex;
+  if (TopLevelDecls)
+DeclsToIndex.assign(TopLevelDecls->begin(), TopLevelDecls->end());
+  else
+DeclsToIndex.assign(AST.getTranslationUnitDecl()->decls().begin(),
+AST.getTranslationUnitDecl()->decls().end());
+
+  index::indexTopLevelDecls(AST, DeclsToIndex, Collector, IndexOpts);
 
   return Collector.takeSymbols();
 }
@@ -81,13 +86,14 @@
 }
 
 void FileIndex::update(PathRef Path, ASTContext *AST,
-   std::shared_ptr PP) {
+   std::shared_ptr PP,
+   llvm::Optional> TopLevelDecls) {
   if (!AST) {
 FSymbols.update(Path, nullptr);
   } else {
 assert(PP);
 auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, URISchemes);
+*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
 FSymbols.update(Path, std::move(Slab));
   }
   auto Symbols = FSymbols.allSymbols();
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -99,6 +99,7 @@
   /// synchronize access to shared state.
   ClangdServer(GlobalCompilationDatabase &CDB, FileSystemProvider &FSProvider,
DiagnosticsConsumer &DiagConsumer, const Options &Opts);
+  ~ClangdServer();
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
@@ -200,6 +201,7 @@
   formatCode(llvm::StringRef Code, PathRef File,
  ArrayRef Ranges);
 
+  class DynamicIndex;
   typedef uint64_t DocVersion;
 
   void consumeDiagnostics(PathRef File, DocVersion Version,
@@ -217,15 +219,14 @@
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
-  //   - the dynamic index owned by ClangdServer (FileIdx)
+  //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
-  // If present, an up-to-date of

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:70
+// FIXME(ibiryukov): this should be a generic helper instead.
+class NoopCallbacks : public ParsingCallbacks {
 public:

ioeric wrote:
> Maybe provide noop implementations for `ParsingCallbacks::onPreambleAST()` 
> and `ParsingCallbacks::onMainAST` by default?
I'll address this in a parent revision. 



Comment at: clangd/ClangdServer.h:246
+  /// If present, an up-to-date of symbols in open files. Read via Index.
+  std::unique_ptr FileIdx;
   // If present, a merged view of FileIdx and an external index. Read via 
Index.

ioeric wrote:
> nit: `s/FileIdx/DymIdx`? (also need to update the comment above)
Went with `DynamicIdx`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



___
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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50993#1207464, @Dmitry.Kozhevnikov wrote:

> Do you think it would be OK to have pthreads-only version there, with 
> `std::thread` fallback on Windows? I'd like to avoid writing a Windows-only 
> implementation myself, as it's a bit alien for me.


Yeah, we should probably do both Unix and Windows there. My guess is that the 
Windows implementation shouldn't be too hard, since most of the code is already 
there.
In any way, it's fine to start with a Unix-only version and a stub in Windows 
and figuring out the implementation during the review. Happy to look into 
Windows myself, it shouldn't be too hard :-)

>> 2. I wonder if we should expose the stack size as an option to clangd?
> 
> Do you expect it would be ever used?

Not frequently, but I don't see how we can figure out good defaults there. It's 
one of those things where the guideline is "as small as possible, as long as it 
does not crash clangd".
An option could provide a simple workaround for cases when clangd is actually 
crashing.


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] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Provide noop callbacks implementation by default.
- Update docs.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847

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

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -17,10 +17,11 @@
 
 namespace clang {
 namespace clangd {
+namespace {
 
 using ::testing::_;
-using ::testing::Each;
 using ::testing::AnyOf;
+using ::testing::Each;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -44,8 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,8 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true,
-/*PreambleParsedCallback=*/nullptr,
+/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -130,8 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
+  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -162,8 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true,
-  /*PreambleParsedCallback=*/nullptr,
+  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -261,7 +258,7 @@
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -313,7 +310,7 @@
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  PreambleParsedCallback(),
+  noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -346,7 +343,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -399,7 +396,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -430,5 +427,6 @@
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
 }
 
+} // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -51,6 +51,30 @@
   unsigned MaxRetainedASTs = 3;
 };
 
+class ParsingCallbacks {
+public:
+  virtual ~ParsingCallbacks() = default;
+
+  /// Called on the AST that was built for emitting the preamble. The built AST
+  /// contains only AST nodes from the #include directives at the start of the
+  /// file. AST node in the current file should be observed on onMainAST call.
+  virtual void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) {}
+  /// Called on the AST built for the file itself. Note that preamble AST nodes
+  /// are not deserialized and should be processed in the onPreambleAST call
+  /// instead.
+  /// The \p AST always contains all AST nodes for the main file itself, an

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:73
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {

hokein wrote:
> I think `Ctx` should be a `pointer` which gives us a way (passing a nullptr) 
> to clean up the `FileIndex`, the same as `ParsedAST` below.
> 
> And I discover that we don't cleanup dynamic index when a file is being 
> closed, is it expected or a bug?
I suggest we add an extra method to `DynamicIndex` that we call when the file 
is closed. I don't think it's intentional that we don't clean up the index for 
the closed files.
Not sure what's the best way to handle invalid ASTs, but we're never calling 
the current callback with `nullptr` anyway, so I suggest we keep it a reference 
to better reflect that callbacks don't actually handle any errors for us.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Use noop callbacks from the parent revision


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h

Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -64,7 +64,11 @@
   /// nullptr, this removes all symbols in the file.
   /// If \p AST is not null, \p PP cannot be null and it should be the
   /// preprocessor that was used to build \p AST.
-  void update(PathRef Path, ASTContext *AST, std::shared_ptr PP);
+  /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all
+  /// top level decls obtained from \p AST are indexed.
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);
 
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
@@ -86,8 +90,12 @@
 /// Retrieves namespace and class level symbols in \p AST.
 /// Exposed to assist in unit tests.
 /// If URISchemes is empty, the default schemes in SymbolCollector will be used.
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
-llvm::ArrayRef URISchemes = {});
+/// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all top
+/// level decls obtained from \p AST are indexed.
+SymbolSlab
+indexAST(ASTContext &AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None,
+ llvm::ArrayRef URISchemes = {});
 
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -8,15 +8,16 @@
 //===--===//
 
 #include "FileIndex.h"
-#include "SymbolCollector.h"
 #include "../Logger.h"
+#include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 namespace clangd {
 
 SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
+llvm::Optional> TopLevelDecls,
 llvm::ArrayRef URISchemes) {
   SymbolCollector::Options CollectorOpts;
   // FIXME(ioeric): we might also want to collect include headers. We would need
@@ -38,10 +39,14 @@
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
 
-  std::vector TopLevelDecls(
-  AST.getTranslationUnitDecl()->decls().begin(),
-  AST.getTranslationUnitDecl()->decls().end());
-  index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+  std::vector DeclsToIndex;
+  if (TopLevelDecls)
+DeclsToIndex.assign(TopLevelDecls->begin(), TopLevelDecls->end());
+  else
+DeclsToIndex.assign(AST.getTranslationUnitDecl()->decls().begin(),
+AST.getTranslationUnitDecl()->decls().end());
+
+  index::indexTopLevelDecls(AST, DeclsToIndex, Collector, IndexOpts);
 
   return Collector.takeSymbols();
 }
@@ -81,13 +86,14 @@
 }
 
 void FileIndex::update(PathRef Path, ASTContext *AST,
-   std::shared_ptr PP) {
+   std::shared_ptr PP,
+   llvm::Optional> TopLevelDecls) {
   if (!AST) {
 FSymbols.update(Path, nullptr);
   } else {
 assert(PP);
 auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, URISchemes);
+*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
 FSymbols.update(Path, std::move(Slab));
   }
   auto Symbols = FSymbols.allSymbols();
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -99,6 +99,7 @@
   /// synchronize access to shared state.
   ClangdServer(GlobalCompilationDatabase &CDB, FileSystemProvider &FSProvider,
DiagnosticsConsumer &DiagConsumer, const Options &Opts);
+  ~ClangdServer();
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
@@ -200,6 +201,7 @@
   formatCode(llvm::StringRef Code, PathRef File,
  ArrayRef Ranges);
 
+  class DynamicIndex;
   typedef uint64_t DocVersion;
 
   void consumeDiagnostics(PathRef File, DocVersion Version,
@@ -217,15 +219,14 @@
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
-  //   - the dynamic index owned by ClangdServer (FileIdx)
+  //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
-  // If present, an up-to-date of symbols in open files. Read via Index.
-  std::unique_ptr FileIdx;
-  /// Callbacks responsible for upd

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

All the issues should be addressed at this point




Comment at: clangd/ClangdServer.cpp:70
+// FIXME(ibiryukov): this should be a generic helper instead.
+class NoopCallbacks : public ParsingCallbacks {
 public:

ilya-biryukov wrote:
> ioeric wrote:
> > Maybe provide noop implementations for `ParsingCallbacks::onPreambleAST()` 
> > and `ParsingCallbacks::onMainAST` by default?
> I'll address this in a parent revision. 
Done


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clangd/index/FileIndex.h:70
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);

hokein wrote:
> Any strong reason to unify the interface (and `indexAST`)? 
> 
> It seems to me that we do different things on `PreambleAST` and `MainAST`:
> 
> - on `PreambleAST`, we only index `Symbols` for code completion.
> - on `MainAST`, we index `Symbols` (to collect other information missing from 
> `PreambleAST`, e.g. definition location), and collect symbol references.
> 
> Therefore, we'd need to set different options for `SymbolCollector` 
> accordingly. Having a single interface would make the implementation tricky 
> (checking `TopLevelDecls` to see whether this is a `PreambleASt`).  
The only reason is to keep the changes to `FileIndex` minimal in this change. 
We'll have to change more code to cover those differences, but I'm not sure I 
have enough context on what's going to be different for this change, a change 
that will plug xrefs into clangd is probably a better candidate for those.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50970: [clangd] Implement BOOST iterator

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looking at this one. (to make sure we don't clash with @ioeric)




Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:111
 
+  const static unsigned DEFAULT_BOOST_SCORE;
+

Maybe make it `constexpr` and put the value into the header?


https://reviews.llvm.org/D50970



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

[PATCH] D50970: [clangd] Implement BOOST iterator

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:92
+  /// function, the parameter is needed to propagate through the tree.
+  virtual unsigned boost(DocID ID) const = 0;
 

Maybe add a note to the comment on why an `ID` parameter actually is here?
IIUC, we need to because various iterators in the tree may point to different 
elements and we need to know which one we've actually matched.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:129
+std::vector>
+consumeAndBoost(Iterator &It,
+size_t Limit = std::numeric_limits::max());

Could we describe the rationale for keeping both `consume` and 
`consumeAndBoost` somewhere in the comments?

From the offline conversation, it seems `consumeAndBoost` is more expensive, 
but our clients will all use it at some point in the future.
The idea of paying for boosting without actually using it seems bad, so keeping 
this function separate makes sense.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:164
+std::unique_ptr createBoost(std::unique_ptr Child,
+  unsigned Factor);
+

Maybe use `float` scores to align with the scoring code we have for completion?


https://reviews.llvm.org/D50970



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

LG, just a few last nits




Comment at: clangd/Cancellation.cpp:30
+
+TaskHandle createTaskHandle() { return std::make_shared(); }
+

NIT: maybe consider inlining it? Since Task has a public default constructor, I 
don't think having this method buys us anything



Comment at: clangd/Cancellation.h:22
+//  function)> Callback) {
+//   // Make sure Taskr is created before starting the thread. Otherwise
+//   // CancellationToken might not get copied into thread.

s/Taskr/Task



Comment at: clangd/Cancellation.h:23
+//   // Make sure Taskr is created before starting the thread. Otherwise
+//   // CancellationToken might not get copied into thread.
+//   auto TH = createTaskHandle();

remove mentions of CancellationToken



Comment at: clangd/Cancellation.h:49
+// The worker code itself (3) should check for cancellations using
+// `CancellationToken` that can be retrieved via
+// `CancellationToken::isCancelled()`.

s/CancellationToken/getCurrentTask().isCancelled()



Comment at: clangd/ClangdLSPServer.cpp:76
+  CancelParams CP;
+  fromJSON(json::Object{{"id", ID}}, CP);
+  return CP.ID;

Maybe extract the relevant code into a helper to avoid creating the unrelated 
class (`CancelParams`)?



Comment at: clangd/ClangdLSPServer.cpp:621
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it != TaskHandles.end()) {
+it->second->cancel();

Invert condition to reduce nesting?
See [LLVM Style 
Guide](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: ioeric, kbobyrev.
Herald added subscribers: kadircet, jfb, arphaman, jkorous, MaskRay.

Replace them with suffix mappings.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088

Files:
  clangd/index/CanonicalIncludes.cpp
  clangd/index/CanonicalIncludes.h

Index: clangd/index/CanonicalIncludes.h
===
--- clangd/index/CanonicalIncludes.h
+++ clangd/index/CanonicalIncludes.h
@@ -41,7 +41,7 @@
   void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
 
   /// Maps all files matching \p RE to \p CanonicalPath
-  void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
+  void addSuffixMapping(llvm::StringRef Suffix, llvm::StringRef CanonicalPath);
 
   /// Sets the canonical include for any symbol with \p QualifiedName.
   /// Symbol mappings take precedence over header mappings.
@@ -55,17 +55,14 @@
 llvm::StringRef QualifiedName) const;
 
 private:
-  // A map from header patterns to header names. This needs to be mutable so
-  // that we can match again a Regex in a const function member.
-  // FIXME(ioeric): All the regexes we have so far are suffix matches. The
-  // performance could be improved by allowing only suffix matches instead of
-  // arbitrary regexes.
-  mutable std::vector>
-  RegexHeaderMappingTable;
-  // A map from fully qualified symbol names to header names.
+  /// A map from full include path to a canonical path.
+  llvm::StringMap FullPathMapping;
+  /// A map from a suffix (one or components of a path) to a canonical path.
+  llvm::StringMap SuffixHeaderMapping;
+  /// Maximum number of path components stored in a key of SuffixHeaderMapping.
+  int MaxSuffixComponents = 0;
+  /// A map from fully qualified symbol names to header names.
   llvm::StringMap SymbolMapping;
-  // Guards Regex matching as it's not thread-safe.
-  mutable std::mutex RegexMutex;
 };
 
 /// Returns a CommentHandler that parses pragma comment on include files to
Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -10,23 +10,29 @@
 #include "CanonicalIncludes.h"
 #include "../Headers.h"
 #include "clang/Driver/Types.h"
-#include "llvm/Support/Regex.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 const char IWYUPragma[] = "// IWYU pragma: private, include ";
 } // namespace
 
-void CanonicalIncludes::addMapping(llvm::StringRef Path,
-   llvm::StringRef CanonicalPath) {
-  addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
-  CanonicalPath);
+void CanonicalIncludes::addSuffixMapping(llvm::StringRef Suffix,
+ llvm::StringRef CanonicalPath) {
+  int Components = 0;
+  for (auto It = llvm::sys::path::begin(Suffix),
+End = llvm::sys::path::end(Suffix);
+   It != End; ++It)
+++Components;
+
+  MaxSuffixComponents = std::max(MaxSuffixComponents, Components);
+  SuffixHeaderMapping[Suffix] = CanonicalPath;
 }
 
-void CanonicalIncludes::addRegexMapping(llvm::StringRef RE,
-llvm::StringRef CanonicalPath) {
-  this->RegexHeaderMappingTable.emplace_back(llvm::Regex(RE), CanonicalPath);
+void CanonicalIncludes::addMapping(llvm::StringRef Path,
+   llvm::StringRef CanonicalPath) {
+  FullPathMapping[Path] = CanonicalPath;
 }
 
 void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
@@ -41,7 +47,6 @@
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
 return SE->second;
-  std::lock_guard Lock(RegexMutex);
   // Find the first header such that the extension is not '.inc', and isn't a
   // recognized non-header file
   auto I =
@@ -62,13 +67,23 @@
   if ((ExtType != driver::types::TY_INVALID) &&
   !driver::types::onlyPrecompileType(ExtType))
 return Headers[0];
-  for (auto &Entry : RegexHeaderMappingTable) {
-#ifndef NDEBUG
-std::string Dummy;
-assert(Entry.first.isValid(Dummy) && "Regex should never be invalid!");
-#endif
-if (Entry.first.match(Header))
-  return Entry.second;
+
+  auto MapIt = FullPathMapping.find(Header);
+  if (MapIt != FullPathMapping.end())
+return MapIt->second;
+
+  int Components = 0;
+  for (auto It = llvm::sys::path::rbegin(Header),
+End = llvm::sys::path::rend(Header);
+   It != End; ++It) {
+++Components;
+if (MaxSuffixComponents < Components)
+  break;
+
+auto SubPath = Header.substr(It->data() - Header.begin());
+auto MappingIt = SuffixHeaderMapping.find(SubPath);
+if (MappingIt != SuffixHeaderMapping.end())
+  return MappingIt->second;
   }
   return Header;
 }
@@ -153,660 +168,660 @@

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.
Herald added a subscriber: kadircet.

Sorry for the delay with this one




Comment at: unittests/clangd/ClangdTests.cpp:1002
+
+  auto MainFileCI = buildCompilerInvocation(PI);
+  auto AST =

Just reuse `PreambleCI`?



Comment at: unittests/clangd/ClangdTests.cpp:1009
+
+  ASSERT_EQ(AST->getDiagnostics().size(), 4u);
+  EXPECT_THAT(AST->getDiagnostics()[0].Message, HasSubstr("preamble1"));

Maybe fold all asserts into one, e.g.:
```EXPECT_THAT(AST->getDiagnostics(), ElementsAre(Field(&Diag::Message, 
HasSubstr("preamble1")), ...)```

Could be made shorter by introducing a matcher for `Field(&Diag::Message, 
HasSubstr(...`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455



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

[PATCH] D51038: [clang] Make sure codecompletion is called for calls even when inside a token.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Maybe add tests? Lit tests for code completion (`clang/test/CodeCompletion`) 
should be a good fit




Comment at: include/clang/Parse/Parser.h:2971
+  /// signature help working even when it is triggered inside a token.
+  bool CalledOverloadCompletion = false;
 };

Maybe put the field closer to the existing ones?



Comment at: lib/Parse/ParseExpr.cpp:2820
 Actions.CodeCompleteOrdinaryName(getCurScope(), Sema::PCC_Expression);
+  CalledOverloadCompletion = true;
   cutOffParsing();

Why should we set this flag here? Can it be handled by `Completer` function 
that we pass into `ParseExperssionList` instead?


Repository:
  rC Clang

https://reviews.llvm.org/D51038



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

[PATCH] D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51077



___
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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

WRT to the configuration argument, using stack size suggested by @arphaman 
seems like a better option. So let's not add any extra options to clangd.




Comment at: clangd/Threading.cpp:76
+
+  if (::pthread_attr_setstacksize(&Attr, 8 * 1024 * 1024) != 0)
+return;

arphaman wrote:
> please use clang::DesiredStackSize instead.
Oh, cool! TIL we have this. Thanks for mentioning it.


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] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D50967



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:73
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr PP) override {

hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > I think `Ctx` should be a `pointer` which gives us a way (passing a 
> > > nullptr) to clean up the `FileIndex`, the same as `ParsedAST` below.
> > > 
> > > And I discover that we don't cleanup dynamic index when a file is being 
> > > closed, is it expected or a bug?
> > I suggest we add an extra method to `DynamicIndex` that we call when the 
> > file is closed. I don't think it's intentional that we don't clean up the 
> > index for the closed files.
> > Not sure what's the best way to handle invalid ASTs, but we're never 
> > calling the current callback with `nullptr` anyway, so I suggest we keep it 
> > a reference to better reflect that callbacks don't actually handle any 
> > errors for us.
> SG, and it is out of scope of this patch. Let's figure it out in the 
> clean-up-index patch.
LG, I'll come up with a follow-up cleanup patch when done with these patches


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50970: [clangd] Implement BOOST iterator

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Summarizing the offline discussion with @kbobyrev, @ioeric and @sammccall in 
two comments.




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

With limit iterator in mind, let's rename this to `consume()` and make it 
non-const.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:137
+/// and BOOST iterators are actually present in the query.
+std::vector>
+consumeAndBoost(Iterator &It,

Let's remove this function and change the interface of consume to return a 
vector of pairs instead.


https://reviews.llvm.org/D50970



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Rebase onto head


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h

Index: clangd/index/FileIndex.h
===
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -64,7 +64,11 @@
   /// nullptr, this removes all symbols in the file.
   /// If \p AST is not null, \p PP cannot be null and it should be the
   /// preprocessor that was used to build \p AST.
-  void update(PathRef Path, ASTContext *AST, std::shared_ptr PP);
+  /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all
+  /// top level decls obtained from \p AST are indexed.
+  void
+  update(PathRef Path, ASTContext *AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None);
 
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
@@ -86,8 +90,12 @@
 /// Retrieves namespace and class level symbols in \p AST.
 /// Exposed to assist in unit tests.
 /// If URISchemes is empty, the default schemes in SymbolCollector will be used.
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
-llvm::ArrayRef URISchemes = {});
+/// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all top
+/// level decls obtained from \p AST are indexed.
+SymbolSlab
+indexAST(ASTContext &AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls = llvm::None,
+ llvm::ArrayRef URISchemes = {});
 
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -8,15 +8,16 @@
 //===--===//
 
 #include "FileIndex.h"
-#include "SymbolCollector.h"
 #include "../Logger.h"
+#include "SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 namespace clangd {
 
 SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
+llvm::Optional> TopLevelDecls,
 llvm::ArrayRef URISchemes) {
   SymbolCollector::Options CollectorOpts;
   // FIXME(ioeric): we might also want to collect include headers. We would need
@@ -38,10 +39,14 @@
   index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   IndexOpts.IndexFunctionLocals = false;
 
-  std::vector TopLevelDecls(
-  AST.getTranslationUnitDecl()->decls().begin(),
-  AST.getTranslationUnitDecl()->decls().end());
-  index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+  std::vector DeclsToIndex;
+  if (TopLevelDecls)
+DeclsToIndex.assign(TopLevelDecls->begin(), TopLevelDecls->end());
+  else
+DeclsToIndex.assign(AST.getTranslationUnitDecl()->decls().begin(),
+AST.getTranslationUnitDecl()->decls().end());
+
+  index::indexTopLevelDecls(AST, DeclsToIndex, Collector, IndexOpts);
 
   return Collector.takeSymbols();
 }
@@ -81,13 +86,14 @@
 }
 
 void FileIndex::update(PathRef Path, ASTContext *AST,
-   std::shared_ptr PP) {
+   std::shared_ptr PP,
+   llvm::Optional> TopLevelDecls) {
   if (!AST) {
 FSymbols.update(Path, nullptr);
   } else {
 assert(PP);
 auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, URISchemes);
+*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
 FSymbols.update(Path, std::move(Slab));
   }
   auto Symbols = FSymbols.allSymbols();
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -99,6 +99,7 @@
   /// synchronize access to shared state.
   ClangdServer(GlobalCompilationDatabase &CDB, FileSystemProvider &FSProvider,
DiagnosticsConsumer &DiagConsumer, const Options &Opts);
+  ~ClangdServer();
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
@@ -200,6 +201,7 @@
   formatCode(llvm::StringRef Code, PathRef File,
  ArrayRef Ranges);
 
+  class DynamicIndex;
   typedef uint64_t DocVersion;
 
   void consumeDiagnostics(PathRef File, DocVersion Version,
@@ -217,15 +219,14 @@
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
-  //   - the dynamic index owned by ClangdServer (FileIdx)
+  //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
-  // If present, an up-to-date of symbols in open files. Read via Index.
-  std::unique_ptr FileIdx;
-  /// Callbacks responsible for updating FileIdx.
-  std::uniq

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clangd/index/CanonicalIncludes.h:44
   /// Maps all files matching \p RE to \p CanonicalPath
-  void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
+  void addSuffixMapping(llvm::StringRef Suffix, llvm::StringRef CanonicalPath);
 

ioeric wrote:
> It seems that this is only file path suffix matching (by components) now. We 
> should probably rename the function to be explicit.
Renamed to addPathSuffixMapping, updated the doc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088



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

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Ilya Biryukov via Phabricator via cfe-commits]

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

- s/addSuffixMapping/addFileSuffixMapping
- Document the intention of MaxSuffixComponents


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088

Files:
  clangd/index/CanonicalIncludes.cpp
  clangd/index/CanonicalIncludes.h

Index: clangd/index/CanonicalIncludes.h
===
--- clangd/index/CanonicalIncludes.h
+++ clangd/index/CanonicalIncludes.h
@@ -40,8 +40,10 @@
   /// Adds a string-to-string mapping from \p Path to \p CanonicalPath.
   void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
 
-  /// Maps all files matching \p RE to \p CanonicalPath
-  void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
+  /// Maps files with last path components matching \p Suffix to \p
+  /// CanonicalPath.
+  void addPathSuffixMapping(llvm::StringRef Suffix,
+llvm::StringRef CanonicalPath);
 
   /// Sets the canonical include for any symbol with \p QualifiedName.
   /// Symbol mappings take precedence over header mappings.
@@ -55,17 +57,15 @@
 llvm::StringRef QualifiedName) const;
 
 private:
-  // A map from header patterns to header names. This needs to be mutable so
-  // that we can match again a Regex in a const function member.
-  // FIXME(ioeric): All the regexes we have so far are suffix matches. The
-  // performance could be improved by allowing only suffix matches instead of
-  // arbitrary regexes.
-  mutable std::vector>
-  RegexHeaderMappingTable;
-  // A map from fully qualified symbol names to header names.
+  /// A map from full include path to a canonical path.
+  llvm::StringMap FullPathMapping;
+  /// A map from a suffix (one or components of a path) to a canonical path.
+  llvm::StringMap SuffixHeaderMapping;
+  /// Maximum number of path components stored in a key of SuffixHeaderMapping.
+  /// Used to reduce the number of lookups into SuffixHeaderMapping.
+  int MaxSuffixComponents = 0;
+  /// A map from fully qualified symbol names to header names.
   llvm::StringMap SymbolMapping;
-  // Guards Regex matching as it's not thread-safe.
-  mutable std::mutex RegexMutex;
 };
 
 /// Returns a CommentHandler that parses pragma comment on include files to
Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -10,23 +10,29 @@
 #include "CanonicalIncludes.h"
 #include "../Headers.h"
 #include "clang/Driver/Types.h"
-#include "llvm/Support/Regex.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 const char IWYUPragma[] = "// IWYU pragma: private, include ";
 } // namespace
 
-void CanonicalIncludes::addMapping(llvm::StringRef Path,
-   llvm::StringRef CanonicalPath) {
-  addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
-  CanonicalPath);
+void CanonicalIncludes::addPathSuffixMapping(llvm::StringRef Suffix,
+ llvm::StringRef CanonicalPath) {
+  int Components = 0;
+  for (auto It = llvm::sys::path::begin(Suffix),
+End = llvm::sys::path::end(Suffix);
+   It != End; ++It)
+++Components;
+
+  MaxSuffixComponents = std::max(MaxSuffixComponents, Components);
+  SuffixHeaderMapping[Suffix] = CanonicalPath;
 }
 
-void CanonicalIncludes::addRegexMapping(llvm::StringRef RE,
-llvm::StringRef CanonicalPath) {
-  this->RegexHeaderMappingTable.emplace_back(llvm::Regex(RE), CanonicalPath);
+void CanonicalIncludes::addMapping(llvm::StringRef Path,
+   llvm::StringRef CanonicalPath) {
+  FullPathMapping[Path] = CanonicalPath;
 }
 
 void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
@@ -41,7 +47,6 @@
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
 return SE->second;
-  std::lock_guard Lock(RegexMutex);
   // Find the first header such that the extension is not '.inc', and isn't a
   // recognized non-header file
   auto I =
@@ -62,13 +67,23 @@
   if ((ExtType != driver::types::TY_INVALID) &&
   !driver::types::onlyPrecompileType(ExtType))
 return Headers[0];
-  for (auto &Entry : RegexHeaderMappingTable) {
-#ifndef NDEBUG
-std::string Dummy;
-assert(Entry.first.isValid(Dummy) && "Regex should never be invalid!");
-#endif
-if (Entry.first.match(Header))
-  return Entry.second;
+
+  auto MapIt = FullPathMapping.find(Header);
+  if (MapIt != FullPathMapping.end())
+return MapIt->second;
+
+  int Components = 0;
+  for (auto It = llvm::sys::path::rbegin(Header),
+End = llvm::sys::path::rend(Header);
+   It != End; ++It) {
+++Components;
+

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Index.cpp:139
+std::sort(Occurrences.begin(), Occurrences.end(),
+  [](const SymbolOccurrence &L, const SymbolOccurrence &R) {
+return L < R;

NIT: remove the lambda? using `<` is the default.



Comment at: clangd/index/Index.cpp:145
+[](const SymbolOccurrence &L, const SymbolOccurrence &R) {
+  return L == R;
+}),

NIT: remove the lambda? Using `==` is the default.



Comment at: clangd/index/Index.cpp:158
+raw_ostream &operator<<(raw_ostream &OS, SymbolOccurrenceKind K) {
+  OS << static_cast(K);
+  return OS;

Is this used for debugging?
In that case maybe consider having a user-readable representation instead of 
the number?



Comment at: clangd/index/Index.h:46
+
+inline bool operator==(const SymbolLocation::Position &L,
+   const SymbolLocation::Position &R) {

NIT: having friend decls inside the classes themselves might prove to be more 
readable.
Not opposed to the current one too, feel free to ignore.



Comment at: clangd/index/Index.h:349
 llvm::DenseMap SymbolIndex;
+
+llvm::DenseMap> SymbolOccurrences;

Maybe add a comment or remove the empty line?



Comment at: clangd/index/Index.h:350
+
+llvm::DenseMap> SymbolOccurrences;
   };

Any store occurences in a file-centric manner?
E.g. 
```
/// Occurences inside a single file.
class FileOccurences {
  StringRef File;
  vector> Locations;
};

// 
DenseMap>  SymbolOccurences;
```

As discussed previously, this representation is better suited for both merging 
and serialization.



Comment at: clangd/index/SymbolCollector.cpp:272
+};
+if (static_cast(Opts.Filter) & Roles) {
+  if (auto ID = getSymbolID(D)) {

NIT: maybe use early exits and inverted conditions to keep the nesting down?



Comment at: clangd/index/SymbolCollector.cpp:321
+
+  const SymbolCollector::Options &CollectorOpts;
+  const SymbolCollector::Options::CollectSymbolOptions &Opts;

If we any `Options` here, why have an extra `CollectorSymbolOptions`?



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

Could you elaborate on what this option will be used for? How do we know in 
advance which symbols we're interested in?


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] D50970: [clangd] Implement BOOST iterator

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D50970



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

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Use std::distance instead of an explicit loop
- Merge break into loop condition


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088

Files:
  clangd/index/CanonicalIncludes.cpp
  clangd/index/CanonicalIncludes.h

Index: clangd/index/CanonicalIncludes.h
===
--- clangd/index/CanonicalIncludes.h
+++ clangd/index/CanonicalIncludes.h
@@ -40,8 +40,10 @@
   /// Adds a string-to-string mapping from \p Path to \p CanonicalPath.
   void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
 
-  /// Maps all files matching \p RE to \p CanonicalPath
-  void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
+  /// Maps files with last path components matching \p Suffix to \p
+  /// CanonicalPath.
+  void addPathSuffixMapping(llvm::StringRef Suffix,
+llvm::StringRef CanonicalPath);
 
   /// Sets the canonical include for any symbol with \p QualifiedName.
   /// Symbol mappings take precedence over header mappings.
@@ -55,17 +57,15 @@
 llvm::StringRef QualifiedName) const;
 
 private:
-  // A map from header patterns to header names. This needs to be mutable so
-  // that we can match again a Regex in a const function member.
-  // FIXME(ioeric): All the regexes we have so far are suffix matches. The
-  // performance could be improved by allowing only suffix matches instead of
-  // arbitrary regexes.
-  mutable std::vector>
-  RegexHeaderMappingTable;
-  // A map from fully qualified symbol names to header names.
+  /// A map from full include path to a canonical path.
+  llvm::StringMap FullPathMapping;
+  /// A map from a suffix (one or components of a path) to a canonical path.
+  llvm::StringMap SuffixHeaderMapping;
+  /// Maximum number of path components stored in a key of SuffixHeaderMapping.
+  /// Used to reduce the number of lookups into SuffixHeaderMapping.
+  int MaxSuffixComponents = 0;
+  /// A map from fully qualified symbol names to header names.
   llvm::StringMap SymbolMapping;
-  // Guards Regex matching as it's not thread-safe.
-  mutable std::mutex RegexMutex;
 };
 
 /// Returns a CommentHandler that parses pragma comment on include files to
Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -10,23 +10,26 @@
 #include "CanonicalIncludes.h"
 #include "../Headers.h"
 #include "clang/Driver/Types.h"
-#include "llvm/Support/Regex.h"
+#include "llvm/Support/Path.h"
+#include 
 
 namespace clang {
 namespace clangd {
 namespace {
 const char IWYUPragma[] = "// IWYU pragma: private, include ";
 } // namespace
 
-void CanonicalIncludes::addMapping(llvm::StringRef Path,
-   llvm::StringRef CanonicalPath) {
-  addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
-  CanonicalPath);
+void CanonicalIncludes::addPathSuffixMapping(llvm::StringRef Suffix,
+ llvm::StringRef CanonicalPath) {
+  int Components = std::distance(llvm::sys::path::begin(Suffix),
+ llvm::sys::path::end(Suffix));
+  MaxSuffixComponents = std::max(MaxSuffixComponents, Components);
+  SuffixHeaderMapping[Suffix] = CanonicalPath;
 }
 
-void CanonicalIncludes::addRegexMapping(llvm::StringRef RE,
-llvm::StringRef CanonicalPath) {
-  this->RegexHeaderMappingTable.emplace_back(llvm::Regex(RE), CanonicalPath);
+void CanonicalIncludes::addMapping(llvm::StringRef Path,
+   llvm::StringRef CanonicalPath) {
+  FullPathMapping[Path] = CanonicalPath;
 }
 
 void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
@@ -41,7 +44,6 @@
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
 return SE->second;
-  std::lock_guard Lock(RegexMutex);
   // Find the first header such that the extension is not '.inc', and isn't a
   // recognized non-header file
   auto I =
@@ -62,13 +64,19 @@
   if ((ExtType != driver::types::TY_INVALID) &&
   !driver::types::onlyPrecompileType(ExtType))
 return Headers[0];
-  for (auto &Entry : RegexHeaderMappingTable) {
-#ifndef NDEBUG
-std::string Dummy;
-assert(Entry.first.isValid(Dummy) && "Regex should never be invalid!");
-#endif
-if (Entry.first.match(Header))
-  return Entry.second;
+
+  auto MapIt = FullPathMapping.find(Header);
+  if (MapIt != FullPathMapping.end())
+return MapIt->second;
+
+  int Components = 1;
+  for (auto It = llvm::sys::path::rbegin(Header),
+End = llvm::sys::path::rend(Header);
+   It != End && Components <= MaxSuffixComponents; ++It, ++Components) {
+au

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/CanonicalIncludes.cpp:24
+  int Components = 0;
+  for (auto It = llvm::sys::path::begin(Suffix),
+End = llvm::sys::path::end(Suffix);

ioeric wrote:
> Would `int Components = begin(Suffix)  - end(Suffix);` work here?
Surprisingly, `operator -` gives byte difference.
But `std::distance` gives the expected result, added that.

Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088



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

[PATCH] D51038: [clang] Make sure codecompletion is called for calls even when inside a token.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry, missed the dependent revision adding tests to clangd at first. Having 
another test in sema would still be great, to make sure we guard against 
regressions if someone does not have clang-tools-extra checked out.


Repository:
  rC Clang

https://reviews.llvm.org/D51038



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

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, kbobyrev, sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay.

The new mode avoids serializing and deserializing YAML.
This results in better performance and less memory usage. Reduce phase
is now almost instant.

The default is to use the old mode going through YAML serialization to
allow migrating MapReduce clients that require the old mode to operate
properly. After we migrate the clients, we can switch the default to
the new mode.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51155

Files:
  clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

Index: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -49,9 +49,34 @@
"not given, such headers will have relative paths."),
 llvm::cl::init(""));
 
+static llvm::cl::opt MergeOnTheFly(
+"merge-on-the-fly",
+llvm::cl::desc(
+"Merges symbols for each processed translation unit as soon "
+"they become available. This results in a smaller memory "
+"usage and an almost instant reduce stage. Optimal for running as a "
+"standalone tool, but cannot be used inside MapReduce."),
+/// FIXME(ibiryukov): change the default to true after updating usages that
+/// rely on MR use-case to work properly.
+llvm::cl::init(false));
+
+/// Responsible for aggregating symbols from each processed file and producing
+/// the final results. All methods in this class must be thread-safe,
+/// 'consumeSymbols' may be called from multiple threads.
+class SymbolsConsumer {
+public:
+  virtual ~SymbolsConsumer() = default;
+
+  /// Consume a SymbolSlab build for a file.
+  virtual void consumeSymbols(SymbolSlab Symbols) = 0;
+  /// Produce a resulting symbol slab, by combining  occurrences of the same
+  /// symbols across translation units.
+  virtual SymbolSlab mergeResults() = 0;
+};
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
+  SymbolIndexActionFactory(SymbolsConsumer &Consumer) : Consumer(Consumer) {}
 
   clang::FrontendAction *create() override {
 // Wraps the index action and reports collected symbols to the execution
@@ -61,10 +86,10 @@
   WrappedIndexAction(std::shared_ptr C,
  std::unique_ptr Includes,
  const index::IndexingOptions &Opts,
- tooling::ExecutionContext *Ctx)
+ SymbolsConsumer &Consumer)
   : WrapperFrontendAction(
 index::createIndexingAction(C, Opts, nullptr)),
-Ctx(Ctx), Collector(C), Includes(std::move(Includes)),
+Consumer(Consumer), Collector(C), Includes(std::move(Includes)),
 PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
 
   std::unique_ptr
@@ -91,14 +116,11 @@
   return;
 }
 
-auto Symbols = Collector->takeSymbols();
-for (const auto &Sym : Symbols) {
-  Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
-}
+Consumer.consumeSymbols(Collector->takeSymbols());
   }
 
 private:
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
   std::shared_ptr Collector;
   std::unique_ptr Includes;
   std::unique_ptr PragmaHandler;
@@ -118,30 +140,72 @@
 CollectorOpts.Includes = Includes.get();
 return new WrappedIndexAction(
 std::make_shared(std::move(CollectorOpts)),
-std::move(Includes), IndexOpts, Ctx);
+std::move(Includes), IndexOpts, Consumer);
   }
 
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
 };
 
-// Combine occurrences of the same symbol across translation units.
-SymbolSlab mergeSymbols(tooling::ToolResults *Results) {
-  SymbolSlab::Builder UniqueSymbols;
-  llvm::BumpPtrAllocator Arena;
-  Symbol::Details Scratch;
-  Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) {
-Arena.Reset();
-llvm::yaml::Input Yin(Value, &Arena);
-auto Sym = clang::clangd::SymbolFromYAML(Yin, Arena);
-clang::clangd::SymbolID ID;
-Key >> ID;
-if (const auto *Existing = UniqueSymbols.find(ID))
-  UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
-else
-  UniqueSymbols.insert(Sym);
-  });
-  return std::move(UniqueSymbols).build();
-}
+/// Stashes per-file results inside ExecutionContext, merges all of them at the
+/// end. Useful for running on MapReduce infrastructure to avoid keeping symbols
+/// from multiple files in memory.
+class ToolExecutorConsumer : public SymbolsConsumer {
+public:
+  ToolExecutorConsumer(ToolExecutor &Executor) : Executor(Executor) {}
+
+  void

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Collecting the timings to build the index for LLVM now. Will publish when 
they're ready


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51155



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