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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.
Herald added a subscriber: kadircet.

I hadn't marked it as done because without symbols in main files I found it 
quite lacking.


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] D50703: [clangd] NFC: Mark Workspace Symbol feature complete in the documentation

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D50703#1205109, @kbobyrev wrote:

> In https://reviews.llvm.org/D50703#1205049, @malaperle wrote:
>
> > I hadn't marked it as done because without symbols in main files I found it 
> > quite lacking.
>
>
> Ah, I see, thank you for spotting it! I was under the impression that it's 
> feature-complete. Should I mark it "Partial" until this is fixed then?


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


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] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

Can there be an option for this? This seems very library specific and could 
break other code bases. Ideally, there would be a generic mechanism for this 
kind of filtering, i.e. specify a pattern of excluded files or symbol names. 
But I understand this would be cumbersome because you want to filter only 
*some* symbol names in *some* files, so it would be difficult for users to 
specify this intersection of conditions on command-line arguments, for example. 
I think this needs to be discussed a bit more or have this turned off by 
default (with an option to turn on!) until there is a more general solution for 
this kind of filtering.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D46751#1095923, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D46751#1095894, @malaperle wrote:
>
> > Can there be an option for this? This seems very library specific and could 
> > break other code bases. Ideally, there would be a generic mechanism for 
> > this kind of filtering, i.e. specify a pattern of excluded files or symbol 
> > names. But I understand this would be cumbersome because you want to filter 
> > only *some* symbol names in *some* files, so it would be difficult for 
> > users to specify this intersection of conditions on command-line arguments, 
> > for example. I think this needs to be discussed a bit more or have this 
> > turned off by default (with an option to turn on!) until there is a more 
> > general solution for this kind of filtering.
>
>
> Having user-configurable filtering may certainly be useful, but requires some 
> design to get right.
>  And even if we have it, I think there's value in automatically handling 
> popular frameworks, unless we know it might break other code **in practice**.


Here :)
http://www.sidefx.com/docs/hdk/_s_o_p___bone_capture_lines_8proto_8h_source.html

> I feel it's better to have the filtering for protos **enabled** by default

I like the idea that things work "out of the box", we have to make sure that it 
doesn't make it buggy for certain code bases and impossible to work around.

In https://reviews.llvm.org/D46751#1095926, @ilya-biryukov wrote:

> So, the first line of the file generated by proto compiler seems to be 
> something like this:
>
>   // Generated by the protocol buffer compiler.  DO NOT EDIT!
>
> If we check the symbol comes from a file with this comment, there will be 
> zero chance that we guess it wrong.
>  And we can always filter, in addition to the user-provided filters that 
> @malaperle proposed (which also sound like a very useful feature to me!)
>
> @ioeric, @malaperle, @sammccall, WDYT?


I think this is good if that's true that the comment is always there. I think 
it would be OK for this to be enabled by default, with a general option to turn 
heuristics off. Not sure what to call it... `-use-symbol-filtering-heuristics` 
:)

If handling for other libraries is added later it would be good to split out 
this code a bit. A collection of "filters" could be passed to symbol collector. 
Each filter/framework-handling could be in it's own source file. Later...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

[PATCH] D44954: [clangd] [RFC] Add more symbols to the index

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 146391.
malaperle added a comment.
Herald added a subscriber: jkorous.

Rework to include member vars/functions and scoped enumerators.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(InScopedEnum, InScopedEnum, "") {
+  return arg.InScopedEnum == InScopedEnum;
+}
 
 namespace clang {
 namespace clangd {
@@ -198,25 +201,28 @@
 } // namespace foo
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAreArray(
+  {QName("Foo"), QName("Foo::f"), QName("f1"), QName("f2"),
+   QName("KInt"), QName("kStr"), QName("foo"), QName("foo::bar"),
+   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
+   QName("foo::bar::v2"), QName("foo::baz")}));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T x = 0;};
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
 template <> struct Tmpl {};
 extern template struct Tmpl;
 template struct Tmpl;
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
-   QName("Tmpl"), DeclRange(Header.range()))}));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAreArray(
+  {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+   AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
 }
 
 TEST_F(SymbolCollectorTest, Locations) {
@@ -334,29 +340,30 @@
   Green
 };
 enum class Color2 {
-  Yellow // ignore
+  Yellow
 };
 namespace ns {
 enum {
   Black
 };
 }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-QName("Green"), QName("Color2"),
-QName("ns"), QName("ns::Black")));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("Red"), QName("Color"), QName("Green"),
+   QName("Color2"), QName("Color2::Yellow"),
+   QName("ns"), QName("ns::Black")));
 }
 
-TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
+TEST_F(SymbolCollectorTest, NamelessSymbols) {
   const std::string Header = R"(
 struct {
   int a;
 } Foo;
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"),
+QName("(anonymous struct)::a")));
 }
 
 TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
@@ -417,7 +424,7 @@
   UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2")));
 }
 
-TEST_F(SymbolCollectorTest, IgnoreClassMembers) {
+TEST_F(SymbolCollectorTest, ClassMembers) {
   const std::string Header = R"(
 class Foo {
   void f() {}
@@ -432,7 +439,10 @@
 void Foo::ssf() {}
   )";
   runSymbolCollector(Header, Main);
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
+   QName("Foo::g"), QName("Foo::sf"),
+   QName("Foo::ssf"), QName("Foo::x")));
 }
 
 TEST_F(SymbolCollectorTest, Scopes) {
@@ -531,6 +541,7 @@
   End:
 Line: 1
 Column: 1
+InScopedEnum:true
 CompletionLabel:'Foo1-label'
 CompletionFilterText:'filter'
 CompletionPlainInsertText:'plain'
@@ -555,6 +566,7 @@
   End:
 Line: 1
 Column: 1
+InScopedEnum:false
 CompletionLabel:'Foo2-label'
 CompletionFilterText:'filter'
 CompletionPlainInsertText:'plain'
@@ -565,13 +577,15 @@
   auto Symbols1 = SymbolsFromYAML(YAML1);
 
   EXPECT_THAT(Symbols1,
-

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/CodeComplete.cpp:932
 Req.Query = Filter->pattern();
+Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+Decl::Kind::LinkageSpec, Decl::Kind::Enum};

I want to add a comment here, but I want to make sure I understand why in the 
first place we were not indexing symbols outside these contexts for the purpose 
of code completion. Is it because those will be available by Sema code 
completion anyway?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D46751#1099097, @ioeric wrote:

> > I think this is good if that's true that the comment is always there. I 
> > think it would be OK for this to be enabled by default, with a general 
> > option to turn heuristics off. Not sure what to call it... 
> > -use-symbol-filtering-heuristics :)
>
> We have other filters in the symbol collector that we think could improve 
> user experience, and we don't provide options for those.


What others filters do you mean? If you mean skipping "members", symbols in 
main files, etc, I a working on making them not skipped, see 
https://reviews.llvm.org/D44954.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D46751#1099235, @sammccall wrote:

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


I agree with all of that. What I don't quite understand is why a flag is not 
ok? Just a fail-safe switch in the mean time? You can even leave it on by 
default so your internal service is not affected. We know for a fact that some 
code bases like Houdini won't work with this, at least there will be an option 
to make it work.

In https://reviews.llvm.org/D46751#1099537, @ioeric wrote:

> In https://reviews.llvm.org/D46751#1099479, @malaperle wrote:
>
> > In https://reviews.llvm.org/D46751#1099097, @ioeric wrote:
> >
> > > > I think this is good if that's true that the comment is always there. I 
> > > > think it would be OK for this to be enabled by default, with a general 
> > > > option to turn heuristics off. Not sure what to call it... 
> > > > -use-symbol-filtering-heuristics :)
> > >
> > > We have other filters in the symbol collector that we think could improve 
> > > user experience, and we don't provide options for those.
> >
> >
> > What others filters do you mean? If you mean skipping "members", symbols in 
> > main files, etc, I a working on making them not skipped, see 
> > https://reviews.llvm.org/D44954.
>
>
> I meant the filters in 
> https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/index/SymbolCollector.cpp#L93
>  e.g. filtering symbols in anonymous namespace, which we think should never 
> appear in the index.


I'll be looking at adding them too. For workspaceSymbols it's useful to be able 
to find them and matches what we had before. But completion will not pull them 
from the index.

> I think members are more interesting than the private proto symbols. We want 
> to un-filter members because there are features that would use them, so 
> indexing them makes sense. But private proto symbols should never be shown to 
> users (e.g. in code completion or workspaceSymbols).
> 
> I also think adding an option for indexing members would actually make more 
> sense because they might significantly increase the index size, and it would 
> be good to have options to disable it if users don't use members (e.g. 
> including members can increase size of our internal global index service, and 
> we are not sure if we are ready for that).

It sounds like we'll need both flags. We should discuss that because I'm 
planning to add even more (almost all?) symbols. I don't think it's common that 
users won't want members for workspaceSymbols though, but I see how this is not 
good for the internal indexing service.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D46751#1099786, @sammccall wrote:

> In https://reviews.llvm.org/D46751#1099633, @malaperle wrote:
>
> > In https://reviews.llvm.org/D46751#1099235, @sammccall wrote:
> >
> > > @malaperle to expand on what Eric said, adding proto hacks with false 
> > > positives and no way to turn them off is indeed not the way to go here!
> > >  There's probably going to be other places we want to filter symbols too, 
> > > and it should probably be extensible/customizable in some way.
> > >  We don't yet have enough examples to know what the structure should be 
> > > (something regex based, a code-plugin system based on `Registry`, or 
> > > something in between), thus the simplest/least invasive option for now 
> > > (it's important for our internal rollout to have *some* mitigation in 
> > > place).
> > >  @ioeric can you add a comment near the proto-filtering stuff indicating 
> > > we should work out how to make this extensible?
> >
> >
> > I agree with all of that. What I don't quite understand is why a flag is 
> > not ok? Just a fail-safe switch in the mean time? You can even leave it on 
> > by default so your internal service is not affected.
>
>
> I think a flag doesn't solve much of the problem, and adds new ones:
>
> - users have to find the flag, and work out how to turn it on in their 
> editor, and (other than embedders) few will bother. And each flag hurts the 
> usability of all the other flags.
> - if this heuristic is usable only sometimes, that's at codebase granularity, 
> not user granularity. Flags don't work that way. (Static index currently has 
> this problem...)
> - these flags end up in config files, so if we later remove the flag we'll 
> *completely* break clangd for such users


I don't really agree with those points, but...

>> We know for a fact that some code bases like Houdini won't work with this, 
>> at least there will be an option to make it work.
> 
> Is this still the case after the last revision (with the comment check?)
>  Agree we should only hardwire this on if we are confident that false 
> positives are vanishingly small.

...I hadn't noticed the latest version. I think it's safe enough in the new 
version that we don't need to discuss this much further until it becomes a 
bigger problem (more libraries, etc).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

@ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make 
sense to add a flag to disable indexing members. Could you comment on that? 
What kind of granularity were you thinking? Would a "member" flag cover both 
class members (member vars and functions) and enum class enumerators for 
example? I think that would be reasonable. But I will also add symbols in main 
files too, so another flag for that? Hmmm.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44954#1102807, @ilya-biryukov wrote:

> A few questions regarding class members. To pinpoint some interesting cases 
> and agree on how we want those to behave in the long run.
>
> How do we handle template specializations? What will the qualified names of 
> those instantiations be?
>  I.e. how do I query for `push_back` inside a vector?  Which of the following 
> queries should produce a result?
>
> - `vector::push_back`. Should it match both `vector::push_back` and 
> `vector::push_back` or only the first one?
> - `vector::push_back`
> - `vector::push_back`


It's probably better to consider this in a future patch. Maybe something like 
the first suggestion: vector::push_back and match both. Otherwise, I would 
think it might be a bit too verbose to have to spell out all of the 
specialization. Maybe we could allow it too. So... all of the above? :)

> What scopes will non-scoped enum members have?
>  E.g. if I have `enum En { A,B,C}`, which of the following queries will and 
> won't find enumerators?
> 
> - `En::A`
> - `::A`
> - `A`

Hmm. I think all of them, since you can refer them like that in code too. Case 
#1 doesn't work but that was the case before this patch so it can probably be 
addressed separately. I'll add some tests though!

In https://reviews.llvm.org/D44954#1103026, @ioeric wrote:

> In https://reviews.llvm.org/D44954#1101922, @malaperle wrote:
>
> > @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make 
> > sense to add a flag to disable indexing members. Could you comment on that? 
> > What kind of granularity were you thinking? Would a "member" flag cover 
> > both class members (member vars and functions) and enum class enumerators 
> > for example? I think that would be reasonable. But I will also add symbols 
> > in main files too, so another flag for that? Hmmm.
>
>
> Sam convinced me that members would still be interesting for our internal 
> index service (e.g. locations of members would be useful for go-to-def). I'll 
> investigate how much impact that would be by running the indexer with your 
> change patched in, but I don't want to block you on that, so I'm fine with 
> checking this in without any filter. We could revisit the filter design when 
> needed.
>
> We actually had an option for indexing symbols in main files :) But it was 
> removed as it turned out to be unused for the features clangd had at that 
> point. I think it would be reasonable to add it back if we start supporting 
> collecting main file symbols again. Maybe out of the scope of this patch, but 
> I am interested in the use cases you have for symbols in main files, besides 
> in `workspaceSymbols`?


It's also for textDocument/documentSymbol. For this, we technically don't need 
them in the static index since we could collect symbols when the document is 
opened, but we also want them for workspaceSymbols so we might as well use the 
same symbol collector, etc. There should be more things that would also use 
symbol in main files, like Type Hierarchy.




Comment at: clangd/CodeComplete.cpp:932
 Req.Query = Filter->pattern();
+Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+Decl::Kind::LinkageSpec, Decl::Kind::Enum};

ilya-biryukov wrote:
> malaperle wrote:
> > I want to add a comment here, but I want to make sure I understand why in 
> > the first place we were not indexing symbols outside these contexts for the 
> > purpose of code completion. Is it because those will be available by Sema 
> > code completion anyway?
> C++ lookup rules inside classes are way more complicated than in namespaces 
> and we can't possibly hope to give a decent approximation for those.
> Moreover, completion inside classes does not require any non-local 
> information, so there does not seem to be a win when using the index anyway.
> So we'd rather rely on clang to do completion there, it will give way more 
> useful results than any index implementation.
Makes sense. Thanks! I'll be able to document this better now with the full 
picture.



Comment at: clangd/index/Index.h:160
+  /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+  Decl::Kind DeclContextKind;
+  /// Whether or not this is an enumerator inside a scoped enum (C++11).

ioeric wrote:
> ilya-biryukov wrote:
> > How do we use `DeclContextKind`?
> > Why did we decide to not go with a `bool ForCompletion` instead? (I'm 
> > probably missing a conversation in the workspaceSymbol review, could you 
> > point me to those instead?)
> > 
> > I'm asking because clang enums are very detailed and designed for use in 
> > the compiler, using them in the index seems to complicate things.
> > It feels we don't need this level of detail in the symbols. Similar to how 
> > we don't store the whole structural type, but rely on string representation 
> > o

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added subscribers: arphaman, malaperle.
malaperle added a comment.

In https://reviews.llvm.org/D47063#1104417, @ilya-biryukov wrote:

> Maybe we can even store enough information to not need the AST for navigation 
> at all and build it only for features like refactorings.
>  @sammccall, let me know what are your thoughts on all of this.


That's what I was thinking. I think I also had similar discussion with 
@arphaman. I think storing a limited number of ASTs is a good interim solution.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44954#1104178, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D44954#1103664, @malaperle wrote:
>
> > It's probably better to consider this in a future patch. Maybe something 
> > like the first suggestion: vector::push_back and match both. Otherwise, I 
> > would think it might be a bit too verbose to have to spell out all of the 
> > specialization. Maybe we could allow it too. So... all of the above? :)
>
>
> This certainly does not have to be addressed in this patch. Just wanted to 
> collect opinions on what the behavior we want in the long term.
>  My thoughts would be towards allowing only `vector::push_back` and make it 
> match both `push_back`s: in `vector` and `vector`.
>  Other cases might work too, but I wouldn't try implementing something that 
> matches specializations. It's just too complicated in the general case. This 
> will only be used in workspaceSymbol and it's fine to give a few more results 
> there...


Sounds very reasonable.

>>> What scopes will non-scoped enum members have?
>> 
>> Hmm. I think all of them, since you can refer them like that in code too. 
>> Case #1 doesn't work but that was the case before this patch so it can 
>> probably be addressed separately. I'll add some tests though!
> 
> I would vote for making queries `En::A` and `A` match the enumerator, but 
> **not** `::A`. The reasoning is: yes, you can reference it this way in a C++ 
> file, but `workspaceSymbol` is not a real C++ resolve and I think it should 
> match the outline of the code rather than the actual C++ lookup rules.
>  E.g. I wouldn't expect it to match symbols from base classes, etc. This 
> should also simplify implementation too.

I don't have a strong opinion, so I can try this suggestion!




Comment at: clangd/index/Index.h:160
+  /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+  Decl::Kind DeclContextKind;
+  /// Whether or not this is an enumerator inside a scoped enum (C++11).

ioeric wrote:
> malaperle wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > How do we use `DeclContextKind`?
> > > > Why did we decide to not go with a `bool ForCompletion` instead? (I'm 
> > > > probably missing a conversation in the workspaceSymbol review, could 
> > > > you point me to those instead?)
> > > > 
> > > > I'm asking because clang enums are very detailed and designed for use 
> > > > in the compiler, using them in the index seems to complicate things.
> > > > It feels we don't need this level of detail in the symbols. Similar to 
> > > > how we don't store the whole structural type, but rely on string 
> > > > representation of completion label instead.
> > > +1
> > > 
> > > `ForCompletion` sounds reasonable as the current design of index-based 
> > > code completion relies on assumptions about contexts. 
> > My thinking was that the "ForCompletion" boolean was too specific and 
> > tailored for one client use. I thought the Symbol information should not 
> > have that much hard-coded knowledge on how it would be used. It would be 
> > odd to have "ForWorkspaceSymbol", "ForDocumentSymbol", etc. That is why I 
> > was going for a slightly more detailed symbol information that opened the 
> > door for more arbitrary queries for symbol clients. But it complicates 
> > things a bit more and I'd be happy to bring back the "ForCompletion" if it 
> > makes more sense for now.
> I think code completion, with the most complicated use of the index so far, 
> probably deserves a flag :P I would expect/hope other features to be less 
> "picky" about symbols. A high-level flag like `ForCompletion` would help keep 
> knowledge about filtering for code completion (e.g. enums ...) inside symbol 
> collector, which I think could be a win.
> 
> FWIW, `ForCompletion` makes it sound like a symbols is collected specifically 
> for code completion. Maybe something like `bool SupportGlobalCompletion` 
> would be better?
> Maybe something like bool SupportGlobalCompletion would be better?

Sounds good!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov, 
klimek.

For enumerators in unscoped enums that have names, even if they are not
scoped, we add the enum name to the scope so that users can find the
enumerators when fully qualifying them, for example: MyEnum::Enumerator.
This makes it more consistent with how the code is visually laid out.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47223

Files:
  clangd/index/SymbolCollector.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -343,9 +343,9 @@
 }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-QName("Green"), QName("Color2"),
-QName("ns"), QName("ns::Black")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+   QName("Red"), QName("Color"), QName("Color::Green"),
+   QName("Color2"), QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
@@ -726,10 +726,10 @@
bool operator<(const TopLevel &, const TopLevel &);
  })";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
-   QName("nx::Kind"), QName("nx::KIND_OK"),
-   QName("nx::operator<")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),
+QName("nx::Kind::KIND_OK"),
+QName("nx::operator<")));
 }
 
 TEST_F(SymbolCollectorTest, DoubleCheckProtoHeaderComment) {
@@ -743,9 +743,9 @@
   }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
-   QName("nx::Kind"), QName("nx::Kind_Fine")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
+QName("nx::Kind"),
+QName("nx::Kind::Kind_Fine")));
 }
 
 } // namespace
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -264,6 +264,21 @@
  match(decl(isExpansionInMainFile()), ND, ND.getASTContext()).empty();
 }
 
+std::string getQualifiedName(const NamedDecl &ND) {
+  std::string QName;
+  llvm::raw_string_ostream OS(QName);
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  // Note that inline namespaces are treated as transparent scopes. This
+  // reflects the way they're most commonly used for lookup. Ideally we'd
+  // include them, but at query time it's hard to find all the inline
+  // namespaces to query: the preamble doesn't have a dedicated list.
+  Policy.SuppressUnwrittenScope = true;
+  ND.printQualifiedName(OS, Policy);
+  OS.flush();
+  assert(!StringRef(QName).startswith("::"));
+  return QName;
+}
+
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -341,21 +356,29 @@
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
-  std::string QName;
-  llvm::raw_string_ostream OS(QName);
-  PrintingPolicy Policy(ASTCtx->getLangOpts());
-  // Note that inline namespaces are treated as transparent scopes. This
-  // reflects the way they're most commonly used for lookup. Ideally we'd
-  // include them, but at query time it's hard to find all the inline
-  // namespaces to query: the preamble doesn't have a dedicated list.
-  Policy.SuppressUnwrittenScope = true;
-  ND.printQualifiedName(OS, Policy);
-  OS.flush();
-  assert(!StringRef(QName).startswith("::"));
+  std::string QName = getQualifiedName(ND);
 
   Symbol S;
   S.ID = std::move(ID);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+
+  using namespace clang::ast_matchers;
+  // For enumerators in unscoped enums that have names, even if they are not
+  // scoped, we add the enum name to the scope so that users can find the
+  // enumerators when fully qualifying them, for example: MyEnum::Enumerator.
+  auto InUnscopedEnum =
+  match(decl(hasDeclContext(enumDecl(unless(isScoped())).bind("enum"))), ND,
+*ASTCtx);
+  std::string EnumQName;
+  if (!InUnscopedEnum.empty()) {
+auto Enum = InUnscopedEnum[0].getNodeAs("enum");
+if (Enum->getDeclName()) {
+  EnumQName = getQualifiedName

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44954#1104497, @malaperle wrote:

> >>> What scopes will non-scoped enum members have?
> >> 
> >> Hmm. I think all of them, since you can refer them like that in code too. 
> >> Case #1 doesn't work but that was the case before this patch so it can 
> >> probably be addressed separately. I'll add some tests though!
> > 
> > I would vote for making queries `En::A` and `A` match the enumerator, but 
> > **not** `::A`. The reasoning is: yes, you can reference it this way in a 
> > C++ file, but `workspaceSymbol` is not a real C++ resolve and I think it 
> > should match the outline of the code rather than the actual C++ lookup 
> > rules.
> >  E.g. I wouldn't expect it to match symbols from base classes, etc. This 
> > should also simplify implementation too.
>
> I don't have a strong opinion, so I can try this suggestion!


I changed the behavior of non-scoped enums as suggested here: 
https://reviews.llvm.org/D47223


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

[PATCH] D47272: [clangd] Build index on preamble changes instead of the AST changes

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

> We do not to rely on symbols from the main file anyway, since any info hat 
> those provide can always be taken from the AST.

I'll be adding those soon for workspace symbols... And also for document 
symbols.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47272



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

[PATCH] D47272: [clangd] Build index on preamble changes instead of the AST changes

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D47272#1109914, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D47272#1109909, @malaperle wrote:
>
> > > We do not to rely on symbols from the main file anyway, since any info 
> > > hat those provide can always be taken from the AST.
> >
> > I'll be adding those soon for workspace symbols... And also for document 
> > symbols.
>
>
> I can add extra code to build pieces for the AST later. This is not hard to 
> do, but would require rearranging some code a bit more.
>  Will try to send the change tomorrow for review tomorrow. Does that SG?


Sounds good. Doesn't have to be tomorrow :) Just making sure we're not on 
incompatible paths.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47272



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 148329.
malaperle added a comment.

Use "SupportGlobalCompletion", white-list decl contexts, add more tests


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(SupportGlobalCompletion, SupportGlobalCompletion, "") {
+  return arg.SupportGlobalCompletion == SupportGlobalCompletion;
+}
 
 namespace clang {
 namespace clangd {
@@ -163,8 +166,20 @@
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   const std::string Header = R"(
 class Foo {
+  Foo() {}
+  Foo(int a) {}
+  void f();
+  friend void f1();
+  friend class Friend;
+  Foo& operator=(const Foo&);
+  ~Foo();
+  class Nested {
   void f();
+  };
 };
+class Friend {
+};
+
 void f1();
 inline void f2() {}
 static const int KInt = 2;
@@ -198,25 +213,68 @@
 } // namespace foo
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+   {QName("Foo"),  QName("Foo::Foo"),
+QName("Foo::Foo"), QName("Foo::f"),
+QName("Foo::~Foo"),QName("Foo::operator="),
+QName("Foo::Nested"),  QName("Foo::Nested::f"),
+QName("Friend"),   QName("f1"),
+QName("f2"),   QName("KInt"),
+QName("kStr"), QName("foo"),
+QName("foo::bar"), QName("foo::int32"),
+QName("foo::int32_t"), QName("foo::v1"),
+QName("foo::bar::v2"), QName("foo::baz")}));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T x = 0;};
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
 template <> struct Tmpl {};
 extern template struct Tmpl;
 template struct Tmpl;
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
-   QName("Tmpl"), DeclRange(Header.range()))}));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAreArray(
+  {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+   AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+  const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+  int foo;
+  ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategory)
+- (void)someMethodName2:(void*)name2;
+@end
+
+@implementation Person (MyCategory)
+- (void)someMethodName2:(void*)name2 {
+  int foo2;
+}
+@end
+
+@protocol MyProtocol
+- (void)someMethodName3:(void*)name3;
+@end
+  )";
+  TestFileName = "test.m";
+  runSymbolCollector(Header, /*Main=*/"", {"-fblocks"});
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(
+  QName("Person"), QName("Person::someMethodName:lastName:"),
+  QName("MyCategory"), QName("Person::someMethodName2:"),
+  QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
 }
 
 TEST_F(SymbolCollectorTest, Locations) {
@@ -334,29 +392,30 @@
   Green
 };
 enum class Color2 {
-  Yellow // ignore
+  Yellow
 };
 namespace ns {
 enum {
   Black
 };
 }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-QName("Green"), QName("Color2"),
-QName("ns"), QName("ns::Black")));
+  EXPECT_THAT(Symbols,
+  Uno

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D47223#1109247, @ilya-biryukov wrote:

> I'm not sure if we have tests for that, but I remember that we kept the 
> enumerators in the outer scope so that completion could find them..
>  Am I right that this patch will change the behavior and we won't get 
> enumerators in the following example:
>
>   /// foo.h
>   enum Foo {
> A, B, C
>   };
>  
>   /// foo.cpp
>   #include "foo.h"
>  
>   int a = ^ // <-- A, B, C should be in completion list here.
>


Not quite but still potentially problematic. With the patch, A, B, C would be 
found but not ::A, ::B, ::C.

> It's one of those cases where code completion and workspace symbol search 
> seem to want different results :-(
>  I suggest to add an extra string field for containing unscoped enum name, 
> maybe into symbol details? And add a parameter to `Index::fuzzyFind` on 
> whether we need to match enum scopes or not.
>  +@ioeric, +@sammccall,  WDYT?

I'll wait to see what others think before changing it. But I feel it's a bit 
odd that completion and workspace symbols would be inconsistent. I'd rather 
have it that A, ::A, and Foo::A work for both completion and workspace. Maybe 
it would complicate things too much...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47223



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added subscribers: ioeric, sammccall, malaperle.
malaperle added a comment.

I think this might have broken the "shared lib" build? Could you double check 
that you don't need to add "clangDriver" ?

../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:61: error: 
undefined reference to 
'clang::driver::types::lookupTypeForExtension(llvm::StringRef)'
../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:63: error: 
undefined reference to 
'clang::driver::types::onlyPrecompileType(clang::driver::types::ID)'

Thanks!



From: cfe-commits  on behalf of Eric Liu 
via Phabricator via cfe-commits 
Sent: Thursday, May 24, 2018 10:44:26 AM
To: ioe...@google.com; sammcc...@google.com
Cc: mask...@google.com; llvm-comm...@lists.llvm.org; cfe-commits@lists.llvm.org
Subject: [PATCH] https://reviews.llvm.org/D47187: [clangd] Skip .inc headers 
when canonicalizing header #include.

This revision was automatically updated to reflect the committed changes.
Closed by commit https://reviews.llvm.org/rL333188: [clangd] Skip .inc headers 
when canonicalizing header #include. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:

  rL LLVM

https://reviews.llvm.org/D47187

Files:

  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp


Repository:
  rL LLVM

https://reviews.llvm.org/D47187



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:158
+  translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+  enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+  objcCategoryImplDecl(), objcImplementationDecl()));

ioeric wrote:
> (Disclaimer: I don't know obj-c...)
> 
> It seems that some objc contexts here are good for global code completion. If 
> we want to support objc symbols, it might also make sense to properly set 
> `SupportGlobalCompletion` for them.
Sounds good. Maybe it would be OK to do this in another (small) patch? I also 
know next to nothing about obj-c :)



Comment at: unittests/clangd/CodeCompleteTests.cpp:741
 
+TEST(CompletionTest, Enums) {
+  EXPECT_THAT(completions(R"cpp(

ioeric wrote:
> It's not clear to me what the following tests (`Enums`, `AnonymousNamespace`, 
> `InMainFile`) are testing. Do they test code completion or  symbol collector? 
> If these test symbol collection, they should be moved int 
> SymbolCollectorTest.cpp
They are testing that code completion works as intended regardless of how 
symbol collector is implemented. It's similar to our previous discussion in 
D44882 about "black box testing". I can remove them but it seems odd to me to 
not have code completion level tests for all cases because we assume that it 
will behave a certain way at the symbol collection and querying levels.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 148834.
malaperle marked 6 inline comments as done.
malaperle added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(Global, SupportGlobalCompletion, "") {
+  return arg.SupportGlobalCompletion == SupportGlobalCompletion;
+}
 
 namespace clang {
 namespace clangd {
@@ -132,9 +135,11 @@
 CollectorOpts, PragmaHandler.get());
 
 std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName,  TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11",   "-include",  TestHeaderName};
 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+
 tooling::ToolInvocation Invocation(
 Args,
 Factory->create(), Files.get(),
@@ -163,8 +168,20 @@
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   const std::string Header = R"(
 class Foo {
+  Foo() {}
+  Foo(int a) {}
   void f();
+  friend void f1();
+  friend class Friend;
+  Foo& operator=(const Foo&);
+  ~Foo();
+  class Nested {
+  void f();
+  };
+};
+class Friend {
 };
+
 void f1();
 inline void f2() {}
 static const int KInt = 2;
@@ -198,25 +215,79 @@
 } // namespace foo
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+   {AllOf(QName("Foo"), Global(true)),
+AllOf(QName("Foo::Foo"), Global(false)),
+AllOf(QName("Foo::Foo"), Global(false)),
+AllOf(QName("Foo::f"), Global(false)),
+AllOf(QName("Foo::~Foo"), Global(false)),
+AllOf(QName("Foo::operator="), Global(false)),
+AllOf(QName("Foo::Nested"), Global(false)),
+AllOf(QName("Foo::Nested::f"), Global(false)),
+
+AllOf(QName("Friend"), Global(true)),
+AllOf(QName("f1"), Global(true)),
+AllOf(QName("f2"), Global(true)),
+AllOf(QName("KInt"), Global(true)),
+AllOf(QName("kStr"), Global(true)),
+AllOf(QName("foo"), Global(true)),
+AllOf(QName("foo::bar"), Global(true)),
+AllOf(QName("foo::int32"), Global(true)),
+AllOf(QName("foo::int32_t"), Global(true)),
+AllOf(QName("foo::v1"), Global(true)),
+AllOf(QName("foo::bar::v2"), Global(true)),
+AllOf(QName("foo::baz"), Global(true))}));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T x = 0;};
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
 template <> struct Tmpl {};
 extern template struct Tmpl;
 template struct Tmpl;
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
-   QName("Tmpl"), DeclRange(Header.range()))}));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAreArray(
+  {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+   AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+  const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+  int foo;
+  ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategor

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: unittests/clangd/SymbolCollectorTests.cpp:141
 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+

This allows to override the "-xc++" with something else, i.e. -xobjective-c++


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314377: [clangd] LSP extension to switch between 
source/header file (authored by malaperle).

Changed prior to commit:
  https://reviews.llvm.org/D36150?vs=116387&id=116919#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36150

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

Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 
+#include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Logger.h"
 #include "clang/Basic/VirtualFileSystem.h"
@@ -899,6 +900,84 @@
   }
 }
 
+TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*SnippetCompletions=*/false, EmptyLogger::getInstance());
+
+  auto SourceContents = R"cpp(
+  #include "foo.h"
+  int b = a;
+  )cpp";
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  auto FooH = getVirtualTestFilePath("foo.h");
+  auto Invalid = getVirtualTestFilePath("main.cpp");
+
+  FS.Files[FooCpp] = SourceContents;
+  FS.Files[FooH] = "int a;";
+  FS.Files[Invalid] = "int main() { \n return 0; \n }";
+
+  llvm::Optional PathResult = Server.switchSourceHeader(FooCpp);
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooH);
+
+  PathResult = Server.switchSourceHeader(FooH);
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooCpp);
+
+  SourceContents = R"c(
+  #include "foo.HH"
+  int b = a;
+  )c";
+
+  // Test with header file in capital letters and different extension, source
+  // file with different extension
+  auto FooC = getVirtualTestFilePath("bar.c");
+  auto FooHH = getVirtualTestFilePath("bar.HH");
+
+  FS.Files[FooC] = SourceContents;
+  FS.Files[FooHH] = "int a;";
+
+  PathResult = Server.switchSourceHeader(FooC);
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooHH);
+
+  // Test with both capital letters
+  auto Foo2C = getVirtualTestFilePath("foo2.C");
+  auto Foo2HH = getVirtualTestFilePath("foo2.HH");
+  FS.Files[Foo2C] = SourceContents;
+  FS.Files[Foo2HH] = "int a;";
+
+  PathResult = Server.switchSourceHeader(Foo2C);
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), Foo2HH);
+
+  // Test with source file as capital letter and .hxx header file
+  auto Foo3C = getVirtualTestFilePath("foo3.C");
+  auto Foo3HXX = getVirtualTestFilePath("foo3.hxx");
+
+  SourceContents = R"c(
+  #include "foo3.hxx"
+  int b = a;
+  )c";
+
+  FS.Files[Foo3C] = SourceContents;
+  FS.Files[Foo3HXX] = "int a;";
+
+  PathResult = Server.switchSourceHeader(Foo3C);
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), Foo3HXX);
+
+  // Test if asking for a corresponding file that doesn't exist returns an empty
+  // string.
+  PathResult = Server.switchSourceHeader(Invalid);
+  EXPECT_FALSE(PathResult.hasValue());
+}
+
 TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
Index: clang-tools-extra/trunk/clangd/ProtocolHandlers.h
===
--- clang-tools-extra/trunk/clangd/ProtocolHandlers.h
+++ clang-tools-extra/trunk/clangd/ProtocolHandlers.h
@@ -49,6 +49,8 @@
 JSONOutput &Out) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
 JSONOutput &Out) = 0;
+  virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput &Out) = 0;  
 };
 
 void regiterCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -248,6 +248,10 @@
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
   Tagged> findDefinitions(PathRef File, Position Pos);
 
+  /// Helper function that returns a path to the corresponding source file when
+  /// given a header file and vice versa.
+  llvm::Optional switchSourceHe

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

I submitted the patch. Thanks a lot William and Ilya!


Repository:
  rL LLVM

https://reviews.llvm.org/D36150



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

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/tool/ClangdMain.cpp:20
 #include 
+#include 
 

William, did you perhaps miss this comment? I don't think unistd.h makes sense 
here.


https://reviews.llvm.org/D37150



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

[PATCH] D38425: [clangd] Document highlights for clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

Just a few quick comments.




Comment at: clangd/ClangdServer.cpp:295
+  assert(FileContents.Draft &&
+ "findDefinitions is called for non-added document");
+

findDocumentHighlights?



Comment at: clangd/ClangdServer.cpp:300
+  std::shared_ptr Resources = Units.getFile(File);
+  assert(Resources && "Calling findDefinitions on non-added file");
+

findDocumentHighlights?



Comment at: clangd/Protocol.cpp:782
+llvm::raw_string_ostream(Result) << llvm::format(
+R"({"range": %s, "number": %d})", Range::unparse(DH.range).c_str(), 
DH.kind);
+return Result;

number -> kind


https://reviews.llvm.org/D38425



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

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Protocol.h:458
+///
+/// A parameter can have a label and a doc-comment.
+struct ParameterInformation {

rwols wrote:
> @malaperle I copied the sentences from the protocol markdown file over 
> [here](https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md),
>  but judging from your comment 
> [here](https://reviews.llvm.org/D35894#inline-314196) that might a problem. 
> Do you suggest to change this or is this okay as-is?
I'm not a lawyer and I have very limited understanding of how these things work 
but I would maybe reword the longer ones and keep it a short summary of a few 
words. I'll ask around what my other open source colleagues think though.


https://reviews.llvm.org/D38048



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdUnit.cpp:81
 
+  std::map takeIncludeMap() {
+return std::move(IncludeMap);

takeIncludeLocationMap?



Comment at: clangd/ClangdUnit.cpp:105
+const SourceManager &SM = CI.getSourceManager();
+unsigned n = SM.local_sloc_entry_size();
+SmallVector InclusionStack;

n -> NumSlocs?



Comment at: clangd/ClangdUnit.cpp:107
+SmallVector InclusionStack;
+std::map::iterator it = IncludeMap.begin();
+

do you need that iterator?



Comment at: clangd/ClangdUnit.cpp:109
+
+for (unsigned i = 0; i < n; ++i) {
+  bool Invalid = false;

i -> I



Comment at: clangd/ClangdUnit.cpp:137
+  FI.getContentCache()->OrigEntry->tryGetRealPathName();
+  if (FilePath.empty()) {
+// FIXME: Does tryGetRealPathName return empty if and only if the path

I think you can just skip it if empty (continue)



Comment at: clangd/ClangdUnit.cpp:143
+  }
+  IncludeMap.insert(std::pair(
+  InclusionStack.front(), FilePath.str()));

I think you can do instead 
IncludeMap.insert({InclusionStack.front(), FilePath.str()});



Comment at: clangd/ClangdUnit.cpp:151
   std::vector TopLevelDeclIDs;
+  std::map IncludeMap;
 };

IncludeMap -> IncludeLocationMap ?



Comment at: clangd/ClangdUnit.cpp:800
 
-  void addDeclarationLocation(const SourceRange &ValSourceRange) {
+  bool isSameLine(unsigned line) const {
+const SourceManager &SourceMgr = AST.getSourceManager();

line -> Line



Comment at: clangd/ClangdUnit.cpp:806
+  void addDeclarationLocation(const SourceRange &ValSourceRange,
+  bool test = false) {
 const SourceManager &SourceMgr = AST.getSourceManager();

remove bool test?



Comment at: clangd/ClangdUnit.cpp:824
+
+  void addLocation(URI uri, Range R) {
+

uri -> Uri



Comment at: clangd/ClangdUnit.cpp:834
+
+for (auto it = IncludeMap.begin(); it != IncludeMap.end(); ++it) {
+  SourceLocation L = it->first;

it -> It



Comment at: clangd/ClangdUnit.cpp:837
+  std::string &Path = it->second;
+  Range r = Range();
+  unsigned line = AST.getSourceManager().getSpellingLineNumber(L);

r -> R



Comment at: clangd/ClangdUnit.cpp:839
+  unsigned line = AST.getSourceManager().getSpellingLineNumber(L);
+  if (isSameLine(line))
+addLocation(URI::fromFile(Path), Range());

line -> Line



Comment at: clangd/ClangdUnit.h:136
   std::vector Diags;
+  std::map IncludeMap;
 };

IncludeLocationMap?



Comment at: clangd/ClangdUnit.h:267
+  clangd::Logger &Logger,
+  std::map);
 

do you want to add a name to the parameter here?



Comment at: unittests/clangd/ClangdTests.cpp:902
 
-TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
   MockFSProvider FS;

the test "CheckSourceHeaderSwitch" was removed


https://reviews.llvm.org/D38639



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#903542, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#895023, @Nebiroth wrote:
>
> > I would like some feedback/suggestions on whether it's possible to "append" 
> > information to a MarkedString to be displayed on client?
>
>
> You mean append **after** returning `MarkedString` to the client? I don't 
> think that's possible.
>  Under what circumstances this could be useful? In general, it seems 
> perfectly fine if we send the whole description+documentation right away.


I think he meant to have multiple sections in the hover, one C/C++ and one not. 
But we noticed you can have an array of MarkedString in Hover so it should be 
fine.


https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D35894#903594, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D35894#903552, @malaperle wrote:
>
> > I think he meant to have multiple sections in the hover, one C/C++ and one 
> > not. But we noticed you can have an array of MarkedString in Hover so it 
> > should be fine.
>
>
> I totally misunderstood the question. Good to know it all works without 
> changing the LSP.
>  Just to make sure I got it right this time: the use-case is to, e.g., return 
> both a code snippet (having `"language": cpp`) and a documentation string 
> (without `language`) as a result of hover request?


Exactly! Although the without-language part, maybe it'll be "scope" information 
first. Maybe documentation/comments in a second patch.


https://reviews.llvm.org/D35894



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle accepted this revision.
malaperle added a comment.

Nice! I had a fix locally for the same purpose but this is much better!


https://reviews.llvm.org/D38939



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

[PATCH] D49523: [clangd] Add support for per-file override compilation command

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D49523#1169000, @arphaman wrote:

> In https://reviews.llvm.org/D49523#1167553, @malaperle wrote:
>
> > Interesting! We also have a need for passing compilation commands in a 
> > context where there is no compile_commands.json, but we were thinking of 
> > putting this in a "didChangeConfiguration" message so that all the commands 
> > would be available even before files are opened. This would be allow Clangd 
> > to have the necessary information for background indexing which would 
> > include unopened files. Subsequent changes to compilation commands would 
> > probably go through a similar didChangeConfiguration and the appropriate 
> > (opened) files would get reparsed (not unlike 
> > https://reviews.llvm.org/D49267). I'm making a few guesses here: I assume 
> > that in the context of XCode, you would not do background indexing in 
> > Clangd but let XCode do it as it can also coordinate (and not overlap) with 
> > build tasks. Is that correct? In any case, I think the approach in the 
> > patch is not incompatible with what we had in mind, i.e. we could also 
> > reuse "overrideCompilationCommandForFile" for each file specified in 
> > didChangeConfiguration. I'm point this out because if you *do* end up 
> > needing all the compilation commands beforehand like I mentioned, then 
> > maybe we can skip the approach of specifying them with didOpen and send 
> > them all with didChangeConfiguration from start.
>
>
> Thanks for your response,
>  As it stands right now we will not run the indexer in Clangd for our use 
> case, and it's unclear if we can even assemble a set of compile commands, so 
> we would like to provide the commands when a file is open. We might be 
> interested in a "didChangeConfiguration" message extension in the future 
> (ideally it would be possible to change the subset of the constructed 
> compilation database).


Sounds good, I think sending it in didOpen makes sense then. And yes, I agree 
that we would need to support changing a subset of commands through 
didChangeConfiguration, eventually!

In https://reviews.llvm.org/D49523#1169620, @jkorous wrote:

> Hi Marc-Andre, what is a structure of data you are passing as parameter of 
> didChangeConfiguration message?


We don't yet :) But we will need to send the information per-file through it 
instead of using the compile_commands.json. Right now, what is supported in the 
didChangeConfiguration is pointing to a different compile_commands.json, in 
order to support multiple configuration (actually pointing to the folder 
containing the json file).

> All we need is to pass per-file compilation command to clangd. Maybe we could 
> send didChangeConfiguration message right after didOpen.
> 
> EDIT: Well, provided we would find a way how not to parse the file twice.

The idea would be to send the per-file commands to clangd *before* anything is 
opened. So the didOpen would just read the latest command in memory. And 
subsequent changes to commands would be communicated with 
didChangeConfiguration and then files would get reparsed. I'm actually thinking 
we might want to send the commands in the "initialize" request for all the 
initial commands and then update them with didChangeConfiguration whenever they 
change. That way, there is no risk for reparsing as we should not do anything 
(indexing!) before the initialize.
But it doesn't sounds like you need this right now :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49523



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

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:430
 CDB.clear();
-
-reparseOpenedFiles();
+compileCommandsChangePost(CCChangeData);
   }

ilya-biryukov wrote:
> Maybe keep the old logic of reparsing all open files? This would make the 
> change way simpler and I don't think we need this extra complexity in the 
> long run, when we have better integration with the build system.
> 
> ClangdServer will reuse the preamble if compile command didn't change anyway, 
> so reparse will be very fast and shouldn't be affected.
> If the compile command does change, we'll retrigger the full rebuild.
I think the change is not that complex but brings much added value. About the 
integration with the build system, there are many build systems out there so I 
don't think better integration will be useful in many scenarios (plain make, 
custom builds, etc). This solution is generic enough so that any build system 
that generates compile_commands.json will be supported in a pretty good way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



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

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.

That way, as soon as the "initialize" is received by the server, it can start
parsing/indexing with a valid compilation database and not have to wait for a
an initial 'didChangeConfiguration' that might or might not happen.
Then, when the user changes configuration, a didChangeConfiguration can be sent.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833

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

Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -322,6 +322,14 @@
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
@@ -348,6 +356,10 @@
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
@@ -419,13 +431,6 @@
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -263,7 +263,7 @@
   O.map("rootPath", R.rootPath);
   O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
-  // initializationOptions, capabilities unused
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -82,6 +82,7 @@
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -72,6 +72,9 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
+  if (Params.initializationOptions)
+applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
 Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
@@ -398,11 +401,8 @@
});
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-DidChangeConfigurationParams &Params) {
-  ClangdConfigurationParamsChange &Settings = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+const ClangdConfigurationParamsChange &Settings) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
 NonCachedCDB.setCompileCommandsDir(
@@ -413,6 +413,13 @@
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+DidChangeConfigurationParams &Params) {
+  ClangdConfigurationParamsChange &Settings = Params.settings;
+  applyConfiguration(Settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
  const clangd::CodeCompleteOptions &CCOpts,
  llvm::Optional CompileCommandsDir,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D49833#1176337, @ilya-biryukov wrote:

> Not strictly opposed to this change, but is there any reason why the clients 
> can't guarantee they'll send didChangeConfiguration right after clangd is 
> initialized?


LSP:

> Until the server has responded to the initialize request with an 
> InitializeResult, the client must not send any additional requests or 
> notifications to the server.

So the client is free to send "didOpen" or any request after the "initialize", 
which means we could end up parsing files with wrong commands before we get the 
first "didChangeConfiguration". In fact, in our indexing prototype we start 
indexing as soon as the "initialize" is processed and we do not know whether or 
not there will be a didChangeConfiguration soon after.
Setting the CDB path as part of the initialize seems more natural and less 
error-prone.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833



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

[PATCH] D49920: [clangd] [WIP] Find references of local symbols

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, arphaman, mgrang, jkorous, MaskRay, 
ioeric, ilya-biryukov.

We do not have a global index of references but we can find the references
of local symbols within the AST in the mean time. Also, since we will not
record local symbol references in the index, we will need that logic anyway.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49920

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -30,6 +30,9 @@
 std::vector findDocumentHighlights(ParsedAST &AST,
   Position Pos);
 
+std::vector findReferences(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration);
+
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional getHover(ParsedAST &AST, Position Pos);
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -211,6 +211,58 @@
   return SymbolID(USR);
 }
 
+/// Finds declarations locations that a given source Decl refers to, in the main
+/// file.
+class ReferenceLocationsFinder : public index::IndexDataConsumer {
+  std::vector ReferenceLocations;
+  ParsedAST *
+  const Decl *ReferencedDecl;
+  index::SymbolRoleSet InterestingRoleSet;
+
+public:
+  ReferenceLocationsFinder(ParsedAST &AST, const Decl *D,
+   bool IncludeDeclaration)
+  : AST(AST), ReferencedDecl(D),
+InterestingRoleSet(
+static_cast(index::SymbolRole::Reference)) {
+if (IncludeDeclaration)
+  InterestingRoleSet |=
+  static_cast(index::SymbolRole::Declaration) |
+  static_cast(index::SymbolRole::Definition);
+  }
+
+  std::vector takeLocations() {
+// Don't keep the same location multiple times.
+// This can happen when nodes in the AST are visited twice.
+std::sort(ReferenceLocations.begin(), ReferenceLocations.end());
+auto last =
+std::unique(ReferenceLocations.begin(), ReferenceLocations.end());
+ReferenceLocations.erase(last, ReferenceLocations.end());
+return std::move(ReferenceLocations);
+  }
+
+  bool
+  handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
+  ArrayRef Relations,
+  SourceLocation Loc,
+  index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+if (D != ReferencedDecl || SourceMgr.isWrittenInMainFile(Loc)) {
+  return true;
+}
+
+// The end loc is adjusted in makeLocation with getLocForEndOfToken.
+SourceRange Range(Loc, Loc);
+
+if (Roles & InterestingRoleSet) {
+  auto L = makeLocation(AST, Range);
+  if (L)
+ReferenceLocations.push_back(*L);
+}
+return true;
+  }
+};
+
 } // namespace
 
 std::vector findDefinitions(ParsedAST &AST, Position Pos,
@@ -324,6 +376,45 @@
   return Result;
 }
 
+std::vector findReferences(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration) {
+  SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+  SourceLocation SourceLocationBeg =
+  getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+
+  DeclarationAndMacrosFinder DeclMacrosFinder(llvm::errs(), SourceLocationBeg,
+  AST.getASTContext(),
+  AST.getPreprocessor());
+  index::IndexingOptions IndexOpts

[PATCH] D49920: [clangd] [WIP] Find references of local symbols

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 157732.
malaperle added a comment.

Fix silly bug I introduced in last minute clean-up.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49920

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test

Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -29,6 +29,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
+# CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -30,6 +30,9 @@
 std::vector findDocumentHighlights(ParsedAST &AST,
   Position Pos);
 
+std::vector findReferences(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration);
+
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional getHover(ParsedAST &AST, Position Pos);
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -211,6 +211,58 @@
   return SymbolID(USR);
 }
 
+/// Finds declarations locations that a given source Decl refers to, in the main
+/// file.
+class ReferenceLocationsFinder : public index::IndexDataConsumer {
+  std::vector ReferenceLocations;
+  ParsedAST *
+  const Decl *ReferencedDecl;
+  index::SymbolRoleSet InterestingRoleSet;
+
+public:
+  ReferenceLocationsFinder(ParsedAST &AST, const Decl *D,
+   bool IncludeDeclaration)
+  : AST(AST), ReferencedDecl(D),
+InterestingRoleSet(
+static_cast(index::SymbolRole::Reference)) {
+if (IncludeDeclaration)
+  InterestingRoleSet |=
+  static_cast(index::SymbolRole::Declaration) |
+  static_cast(index::SymbolRole::Definition);
+  }
+
+  std::vector takeLocations() {
+// Don't keep the same location multiple times.
+// This can happen when nodes in the AST are visited twice.
+std::sort(ReferenceLocations.begin(), ReferenceLocations.end());
+auto last =
+std::unique(ReferenceLocations.begin(), ReferenceLocations.end());
+ReferenceLocations.erase(last, ReferenceLocations.end());
+return std::move(ReferenceLocations);
+  }
+
+  bool
+  handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
+  ArrayRef Relations,
+  SourceLocation Loc,
+  index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+if (D != ReferencedDecl || !SourceMgr.isWrittenInMainFile(Loc)) {
+  return true;
+}
+
+// The end loc is adjusted in makeLocation with getLocForEndOfToken.
+SourceRange Range(Loc, Loc);
+
+if (Roles & InterestingRoleSet) {
+  auto L = makeLocation(AST, Range);
+  if (L)
+ReferenceLocations.push_back(*L);
+}
+return true;
+  }
+};
+
 } // namespace
 
 std::vector findDefinitions(ParsedAST &AST, Position Pos,
@@ -324,6 +376,45 @@
   return Result;
 }
 
+std::vector findReferences(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration) {
+  SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+  SourceLocation SourceLocationBeg =
+  getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+
+  DeclarationAndMacrosFinder DeclMacrosFinder(llvm::errs(), SourceLocationBeg,
+  AST.getASTContext(),
+  AST.getPreprocessor());
+  index::IndexingOptions IndexOpts;
+  IndexOpts.SystemSymbolFilter =
+  index::IndexingOptions::SystemSymbolFilterKind::All;
+  IndexOpts.IndexFunctionLocals = true;
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
+ DeclMacrosFinder, IndexOpts);
+  st

[PATCH] D49920: [clangd] [WIP] Find references of local symbols

[Marc-Andre Laperle via Phabricator via cfe-commits]

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

Needs tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49920



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

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

Was there any objection to this patch? It would have been nice to have this 
before the branching.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833



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

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

@simark would you mind finishing this patch and committing it? I won't be able 
to finish it given that I started it at a different company, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D48903#1187605, @simark wrote:

> If somebody else could run the tests on Windows, it would make me a bit more 
> confident too.


Which tests/targets exactly? If you know


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D48903#1188437, @malaperle wrote:

> In https://reviews.llvm.org/D48903#1187605, @simark wrote:
>
> > If somebody else could run the tests on Windows, it would make me a bit 
> > more confident too.
>
>
> Which tests/targets exactly? If you know


NVM, I saw the "check-clang and check-clang-tools" above.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

Both check-clang and check-clang-tools pass successfully for me on Windows with 
the patch.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#1004937, @ioeric wrote:

> I'm wondering if there is any further plan for this? ;)


I'd like to comment on the amount of data that will be stored but that can be 
done outside this review. I still have a few things to figure out before 
reaching a conclusion.


https://reviews.llvm.org/D39050



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 133978.
malaperle added a comment.
Herald added subscribers: ioeric, jkorous-apple.

Move tests to XRefsTests, change IncludeReferenceMap to a vector and rename it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -50,6 +50,11 @@
   return std::move(*AST);
 }
 
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
+  PathRef File, Tagged> Diagnostics) override {}
+};
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -227,6 +232,59 @@
   }
 }
 
+TEST(GoToInclude, All) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const char *SourceContents = R"cpp(
+  #include "$2^invalid.h"
+  #include "^foo.h"
+  int b = a;
+  // test
+  int foo;
+  #include "$3^foo.h"
+  )cpp";
+  Annotations SourceAnnoations(SourceContents);
+  FS.Files[FooCpp] = SourceAnnoations.code();
+  auto FooH = getVirtualTestFilePath("foo.h");
+
+  const char *HeaderContents = R"cpp([[]]int a;)cpp";
+  Annotations HeaderAnnoations(HeaderContents);
+  FS.Files[FooH] = HeaderAnnoations.code();
+
+  Server.addDocument(FooH, HeaderAnnoations.code());
+  Server.addDocument(FooCpp, SourceAnnoations.code());
+
+  // Test include in preamble
+  auto ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point());
+  ASSERT_TRUE(!!ExpectedLocations);
+  std::vector Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("2"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  // Test include outside of Preamble
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("3"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
 //===-===//
 #include "XRefs.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "URI.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
@@ -129,12 +130,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -167,6 +164,21 @@
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
  DeclMacrosFinder, IndexOpts);
 
+  /// Process targets for paths inside #include directive.
+  std::vector IncludeTargets;
+  for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+  IncludeTargets.push_back(Location{URIForFile{IncludeLoc.second},
+Range{Position{0, 0}, Position{0, 0}}});
+  return IncludeTargets;
+}
+  }
+
   std::vector Decls = DeclMacrosFinder->takeDecls();
   std::vector MacroInfos =
   DeclMacrosFinder->takeMacroInfos();
@@ -242,13 +254,8 @@
   DocumentHighlight getDocumentHighlight(SourceRange SR,
  DocumentHighlightKind Kind) {
 const SourceManager &S

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.h:51
 
+using IncludeReferenceMap = std::unordered_map;
+

ilya-biryukov wrote:
> We use `unordered_map` as a `vector>` here. (i.e. we never look up 
> values by key, because we don't only the range, merely a point in the range)
> Replace map with `vector>` and remove `RangeHash` that we don't need 
> anymore.
Done. I also renamed, IncludeReferenceMap  to InclusionLocations. I thought I 
was more suitable.



Comment at: unittests/clangd/ClangdTests.cpp:781
+
+  Position P = Position{1, 11};
+

ilya-biryukov wrote:
> We moved `findDefinition` tests to `XRefsTests.cpp` and added a useful 
> `Annotation` helper to make writing these tests simpler.
> Maybe we could use it for this test as well?
Done. (I don't know why I can't use the "done" checkbox, maybe because I wasn't 
the original author?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

I haven't looked at the newest patch yet but I gave it a quick try and saw 
something odd. If I change the configuration to something invalid (say I 
specify the path to a CMakeLists.txt), then I get many errors/diagnostics, 
which is normal. But then when I change the config to something valid, the same 
diagnostics re-emitted, as if the configuration failed to change. I'll check 
the code tomorrow a bit but I thought I'd share this with you early.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:53
 
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+  void onDiagnosticsReady(

ilya-biryukov wrote:
> NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.
It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer 
from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that 
wouldn't work.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: unittests/clangd/XRefsTests.cpp:245
+  const char *SourceContents = R"cpp(
+  #include "$2^invalid.h"
+  #include "^foo.h"

ilya-biryukov wrote:
> Could we also add tests for corner cases: cursor before opening quote, cursor 
> after the closing quote, cursor in the middle of `#include` token? (we 
> shouldn't navigate anywhere in the middle of the #include token)
> cursor before opening quote, cursor after the closing quote

I assume we don't want to navigate anywhere for these positions? I don't have 
an opinion.

> (we shouldn't navigate anywhere in the middle of the #include token)

It did in CDT and I thought it worked nicely as it made it easier to click on 
it. You can hold ctrl on the whole line and it underlined it (well except 
trailing comments). But clients don't handle the spaces nicely (underline 
between #include and file name) so I thought I'd work on the client first 
before making the server do it. Anyhow, for now it shouldn't navigate indeed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134296.
malaperle added a comment.

Fix some NITs, add more tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -257,6 +257,89 @@
SourceAnnotations.range()}));
 }
 
+TEST(GoToInclude, All) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const char *SourceContents = R"cpp(
+  #include ^"$2^foo.h$3^"
+  #include "$4^invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #in$5^clude "$6^foo.h"$7^
+  )cpp";
+  Annotations SourceAnnoations(SourceContents);
+  FS.Files[FooCpp] = SourceAnnoations.code();
+  auto FooH = getVirtualTestFilePath("foo.h");
+
+  const char *HeaderContents = R"cpp([[]]int a;)cpp";
+  Annotations HeaderAnnoations(HeaderContents);
+  FS.Files[FooH] = HeaderAnnoations.code();
+
+  Server.addDocument(FooH, HeaderAnnoations.code());
+  Server.addDocument(FooCpp, SourceAnnoations.code());
+
+  // Test include in preamble.
+  auto ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point());
+  ASSERT_TRUE(!!ExpectedLocations);
+  std::vector Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+  // Test include in preamble, last char.
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("2"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("3"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+  // Test include outside of preamble.
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("6"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  ASSERT_EQ(Locations[0].uri.file, FooH);
+  ASSERT_EQ(Locations[0].range, HeaderAnnoations.range());
+
+  // Test a few positions that do not result in Locations.
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("4"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("5"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  ExpectedLocations =
+  Server.findDefinitions(FooCpp, SourceAnnoations.point("7"));
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
 //===-===//
 #include "XRefs.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "URI.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -193,6 +190,15 @@
   Result.push_back(*L);
   }
 
+  /// Process targets for paths inside #include directive.
+  for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.contains(Pos))
+  Result

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134432.
malaperle added a comment.

Use EXPECT_THAT in a few places and clean-ups in tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
 namespace {
 using testing::ElementsAre;
 using testing::Field;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
@@ -259,6 +260,73 @@
SourceAnnotations.range()}));
 }
 
+TEST(GoToInclude, All) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const char *SourceContents = R"cpp(
+  #include ^"$2^foo.h$3^"
+  #include "$4^invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #in$5^clude "$6^foo.h"$7^
+  )cpp";
+  Annotations SourceAnnotations(SourceContents);
+  FS.Files[FooCpp] = SourceAnnotations.code();
+  auto FooH = getVirtualTestFilePath("foo.h");
+  auto FooHUri = URIForFile{FooH.str()};
+
+  const char *HeaderContents = R"cpp([[]]int a;)cpp";
+  Annotations HeaderAnnotations(HeaderContents);
+  FS.Files[FooH] = HeaderAnnotations.code();
+
+  Server.addDocument(FooH, HeaderAnnotations.code());
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+  // Test include in preamble.
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include in preamble, last char.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include outside of preamble.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test a few positions that do not result in Locations.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
 //===-===//
 #include "XRefs.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "URI.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -193,6 +190,15 @@
   Result.push_back(*L);
   }
 
+  /// Process targets for paths inside #include directive.
+  for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134549.
malaperle added a comment.

Change some EXPECT_TRUE to ASSERT_TRUE


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
 namespace {
 using testing::ElementsAre;
 using testing::Field;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
@@ -259,6 +260,73 @@
SourceAnnotations.range()}));
 }
 
+TEST(GoToInclude, All) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const char *SourceContents = R"cpp(
+  #include ^"$2^foo.h$3^"
+  #include "$4^invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #in$5^clude "$6^foo.h"$7^
+  )cpp";
+  Annotations SourceAnnotations(SourceContents);
+  FS.Files[FooCpp] = SourceAnnotations.code();
+  auto FooH = getVirtualTestFilePath("foo.h");
+  auto FooHUri = URIForFile{FooH.str()};
+
+  const char *HeaderContents = R"cpp([[]]int a;)cpp";
+  Annotations HeaderAnnotations(HeaderContents);
+  FS.Files[FooH] = HeaderAnnotations.code();
+
+  Server.addDocument(FooH, HeaderAnnotations.code());
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+  // Test include in preamble.
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include in preamble, last char.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include outside of preamble.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test a few positions that do not result in Locations.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -8,6 +8,7 @@
 //===-===//
 #include "XRefs.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "URI.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
@@ -130,12 +131,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -193,6 +190,15 @@
   Result.push_back(*L);
   }
 
+  /// Process targets for paths inside #include directive.
+  for (auto &IncludeLoc : AST.getInclusionLocations()) {
+Range R = IncludeLoc.first;
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.conta

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134692.
malaperle added a comment.

Rebase.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;
+}
+  )cpp",
+

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 134708.
malaperle added a comment.

Add a missing std::move, fix some formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+  return ^foo(42);
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Field
+struct Foo { int x; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field with initialization
+struct Foo { int x = 5; };
+int main() {
+  Foo bar;
+  bar.^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x = 5",
+  },
+  {
+  R"cpp(// Static field
+struct Foo { static int x; };
+int main() {
+  Foo::^x;
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x",
+  },
+  {
+  R"cpp(// Field, member initializer
+struct Foo {
+  int x;
+  Foo() : ^x(0) {}
+};
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, GNU old-style field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { ^x : 1 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Field, field designator
+struct Foo { int x; };
+int main() {
+  Foo bar = { .^x = 2 };
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x",
+  },
+  {
+  R"cpp(// Method call
+struct Foo { int x(); };
+int main() {
+  Foo bar;
+  bar.^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nint x()",
+  },
+  {
+  R"cpp(// Static method call
+struct Foo { static int x(); };
+int main() {
+  Foo::^x();
+}
+  )cpp",
+  "Declared in struct Foo\n\nstatic int x()",
+  },
+  {
+  R"cpp(// Typedef
+typedef int Foo;
+int main() {
+  ^Foo bar;

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

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

Works well and code looks good. There were only minor tweaks since the last 
approval. I'll land this since there are some issues with Simon's svn account.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE325395: [clangd] Implement textDocument/hover (authored by 
malaperle, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D35894?vs=134708&id=134712#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  test/clangd/hover.test
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/XRefsTests.cpp

Index: test/clangd/initialize-params-invalid.test
===
--- test/clangd/initialize-params-invalid.test
+++ test/clangd/initialize-params-invalid.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: test/clangd/hover.test
===
--- test/clangd/hover.test
+++ test/clangd/hover.test
@@ -0,0 +1,19 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":27}}}
+#  CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"contents": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+} // namespace ns1
+int main() {
+  ns1::My^Union Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nunion MyUnion {}",
+  },
+  {
+  R"cpp(// Function definition via pointer
+int foo(int) {}
+int main() {
+  auto *X = &^foo;
+}
+  )cpp",
+  "Declared in global namespace\n\nint foo(int)",
+  },
+  {
+  R"cpp(// Function declaration via call
+int foo(int);
+int main() {
+ 

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325395: [clangd] Implement textDocument/hover (authored by 
malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D35894

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/XRefs.h
  clang-tools-extra/trunk/test/clangd/hover.test
  clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
  clang-tools-extra/trunk/test/clangd/initialize-params.test
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clang-tools-extra/trunk/test/clangd/hover.test
===
--- clang-tools-extra/trunk/test/clangd/hover.test
+++ clang-tools-extra/trunk/test/clangd/hover.test
@@ -0,0 +1,19 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":27}}}
+#  CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"contents": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:  "clangd.insertInclude"
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
+# CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -258,6 +258,311 @@
   ElementsAre(Location{URIForFile{FooCpp}, SourceAnnotations.range()}));
 }
 
+TEST(Hover, All) {
+  struct OneTest {
+StringRef Input;
+StringRef ExpectedHover;
+  };
+
+  OneTest Tests[] = {
+  {
+  R"cpp(// Local variable
+int main() {
+  int bonjour;
+  ^bonjour = 2;
+  int test1 = bonjour;
+}
+  )cpp",
+  "Declared in function main\n\nint bonjour",
+  },
+  {
+  R"cpp(// Local variable in method
+struct s {
+  void method() {
+int bonjour;
+^bonjour = 2;
+  }
+};
+  )cpp",
+  "Declared in function s::method\n\nint bonjour",
+  },
+  {
+  R"cpp(// Struct
+namespace ns1 {
+  struct MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nstruct MyClass {}",
+  },
+  {
+  R"cpp(// Class
+namespace ns1 {
+  class MyClass {};
+} // namespace ns1
+int main() {
+  ns1::My^Class* Params;
+}
+  )cpp",
+  "Declared in namespace ns1\n\nclass MyClass {}",
+  },
+  {
+  R"cpp(// Union
+namespace ns1 {
+  union MyUnion { int x; int y; };
+ 

[PATCH] D43411: [clangd] Rename some protocol field to lower case

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov, 
klimek.

Also fixes a GCC compilation error.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43411

Files:
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -559,7 +559,7 @@
 auto AST = build(T.code());
 Hover H = getHover(AST, T.point());
 
-EXPECT_EQ(H.Contents.Value, Test.ExpectedHover) << Test.Input;
+EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input;
   }
 }
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -382,9 +382,9 @@
   if (NamedScope) {
 assert(!NamedScope->empty());
 
-H.Contents.Value += "Declared in ";
-H.Contents.Value += *NamedScope;
-H.Contents.Value += "\n\n";
+H.contents.value += "Declared in ";
+H.contents.value += *NamedScope;
+H.contents.value += "\n\n";
   }
 
   // We want to include the template in the Hover.
@@ -401,16 +401,16 @@
 
   OS.flush();
 
-  H.Contents.Value += DeclText;
+  H.contents.value += DeclText;
   return H;
 }
 
 /// Generate a \p Hover object given the macro \p MacroInf.
 static Hover getHoverContents(StringRef MacroName) {
   Hover H;
 
-  H.Contents.Value = "#define ";
-  H.Contents.Value += MacroName;
+  H.contents.value = "#define ";
+  H.contents.value += MacroName;
 
   return H;
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -486,18 +486,18 @@
 };
 
 struct MarkupContent {
-  MarkupKind Kind = MarkupKind::PlainText;
-  std::string Value;
+  MarkupKind kind = MarkupKind::PlainText;
+  std::string value;
 };
 json::Expr toJSON(const MarkupContent &MC);
 
 struct Hover {
   /// The hover's content
-  MarkupContent Contents;
+  MarkupContent contents;
 
   /// An optional range is a range inside a text document
   /// that is used to visualize a hover, e.g. by changing the background color.
-  llvm::Optional Range;
+  llvm::Optional range;
 };
 json::Expr toJSON(const Hover &H);
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -397,20 +397,20 @@
 }
 
 json::Expr toJSON(const MarkupContent &MC) {
-  if (MC.Value.empty())
+  if (MC.value.empty())
 return nullptr;
 
   return json::obj{
-  {"kind", toTextKind(MC.Kind)},
-  {"value", MC.Value},
+  {"kind", toTextKind(MC.kind)},
+  {"value", MC.value},
   };
 }
 
 json::Expr toJSON(const Hover &H) {
-  json::obj Result{{"contents", toJSON(H.Contents)}};
+  json::obj Result{{"contents", toJSON(H.contents)}};
 
-  if (H.Range.hasValue())
-Result["range"] = toJSON(*H.Range);
+  if (H.range.hasValue())
+Result["range"] = toJSON(*H.range);
 
   return std::move(Result);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43411: [clangd] Rename some protocol field to lower case

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325409: [clangd] Rename some protocol field to lower case 
(authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43411

Files:
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -559,7 +559,7 @@
 auto AST = build(T.code());
 Hover H = getHover(AST, T.point());
 
-EXPECT_EQ(H.Contents.Value, Test.ExpectedHover) << Test.Input;
+EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input;
   }
 }
 
Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -486,18 +486,18 @@
 };
 
 struct MarkupContent {
-  MarkupKind Kind = MarkupKind::PlainText;
-  std::string Value;
+  MarkupKind kind = MarkupKind::PlainText;
+  std::string value;
 };
 json::Expr toJSON(const MarkupContent &MC);
 
 struct Hover {
   /// The hover's content
-  MarkupContent Contents;
+  MarkupContent contents;
 
   /// An optional range is a range inside a text document
   /// that is used to visualize a hover, e.g. by changing the background color.
-  llvm::Optional Range;
+  llvm::Optional range;
 };
 json::Expr toJSON(const Hover &H);
 
Index: clang-tools-extra/trunk/clangd/XRefs.cpp
===
--- clang-tools-extra/trunk/clangd/XRefs.cpp
+++ clang-tools-extra/trunk/clangd/XRefs.cpp
@@ -382,9 +382,9 @@
   if (NamedScope) {
 assert(!NamedScope->empty());
 
-H.Contents.Value += "Declared in ";
-H.Contents.Value += *NamedScope;
-H.Contents.Value += "\n\n";
+H.contents.value += "Declared in ";
+H.contents.value += *NamedScope;
+H.contents.value += "\n\n";
   }
 
   // We want to include the template in the Hover.
@@ -401,16 +401,16 @@
 
   OS.flush();
 
-  H.Contents.Value += DeclText;
+  H.contents.value += DeclText;
   return H;
 }
 
 /// Generate a \p Hover object given the macro \p MacroInf.
 static Hover getHoverContents(StringRef MacroName) {
   Hover H;
 
-  H.Contents.Value = "#define ";
-  H.Contents.Value += MacroName;
+  H.contents.value = "#define ";
+  H.contents.value += MacroName;
 
   return H;
 }
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -397,20 +397,20 @@
 }
 
 json::Expr toJSON(const MarkupContent &MC) {
-  if (MC.Value.empty())
+  if (MC.value.empty())
 return nullptr;
 
   return json::obj{
-  {"kind", toTextKind(MC.Kind)},
-  {"value", MC.Value},
+  {"kind", toTextKind(MC.kind)},
+  {"value", MC.value},
   };
 }
 
 json::Expr toJSON(const Hover &H) {
-  json::obj Result{{"contents", toJSON(H.Contents)}};
+  json::obj Result{{"contents", toJSON(H.contents)}};
 
-  if (H.Range.hasValue())
-Result["range"] = toJSON(*H.Range);
+  if (H.range.hasValue())
+Result["range"] = toJSON(*H.range);
 
   return std::move(Result);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43429: [clangd] Add missing library (clangLex) in a few places

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov, 
mgorny, klimek.

Fixes undefined reference to 'clang::Preprocessor::addCommentHandler'
when building with -DBUILD_SHARED_LIBS.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43429

Files:
  clangd/global-symbol-builder/CMakeLists.txt
  unittests/clangd/CMakeLists.txt


Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -38,6 +38,7 @@
   clangFormat
   clangFrontend
   clangIndex
+  clangLex
   clangSema
   clangTooling
   clangToolingCore
Index: clangd/global-symbol-builder/CMakeLists.txt
===
--- clangd/global-symbol-builder/CMakeLists.txt
+++ clangd/global-symbol-builder/CMakeLists.txt
@@ -15,5 +15,6 @@
   clangDaemon
   clangBasic
   clangFrontend
+  clangLex
   clangTooling
 )


Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -38,6 +38,7 @@
   clangFormat
   clangFrontend
   clangIndex
+  clangLex
   clangSema
   clangTooling
   clangToolingCore
Index: clangd/global-symbol-builder/CMakeLists.txt
===
--- clangd/global-symbol-builder/CMakeLists.txt
+++ clangd/global-symbol-builder/CMakeLists.txt
@@ -15,5 +15,6 @@
   clangDaemon
   clangBasic
   clangFrontend
+  clangLex
   clangTooling
 )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43429: [clangd] Add missing library (clangLex) in a few places

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle abandoned this revision.
malaperle added a comment.

Already landed in another commit.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43429



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325662: [clangd] #include statements support for Open 
definition (authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D38639?vs=134549&id=135187#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38639

Files:
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.h
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/SourceCode.h
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -36,6 +36,7 @@
 namespace {
 using testing::ElementsAre;
 using testing::Field;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
@@ -563,6 +564,73 @@
   }
 }
 
+TEST(GoToInclude, All) {
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  const char *SourceContents = R"cpp(
+  #include ^"$2^foo.h$3^"
+  #include "$4^invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #in$5^clude "$6^foo.h"$7^
+  )cpp";
+  Annotations SourceAnnotations(SourceContents);
+  FS.Files[FooCpp] = SourceAnnotations.code();
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+
+  const char *HeaderContents = R"cpp([[]]int a;)cpp";
+  Annotations HeaderAnnotations(HeaderContents);
+  FS.Files[FooH] = HeaderAnnotations.code();
+
+  Server.addDocument(FooH, HeaderAnnotations.code());
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+
+  // Test include in preamble.
+  auto Locations =
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include in preamble, last char.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test include outside of preamble.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value,
+  ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+
+  // Test a few positions that do not result in Locations.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(Locations->Value, IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/ClangdUnit.h
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.h
+++ clang-tools-extra/trunk/clangd/ClangdUnit.h
@@ -45,15 +45,19 @@
   llvm::SmallVector FixIts;
 };
 
+using InclusionLocations = std::vector>;
+
 // Stores Preamble and associated data.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble,
std::vector TopLevelDeclIDs,
-   std::vector Diags);
+   std::vector Diags,
+   InclusionLocations IncLocations);
 
   PrecompiledPreamble Preamble;
   std::vector TopLevelDeclIDs;
   std::vector Diags;
+  InclusionLocations IncLocations;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
@@ -97,13 +101,14 @@
   /// Returns the esitmated size of the AST and the accessory structures, in
   /// bytes. Does not include the size of the preamble.
   std::size_t getUsedBytes() const;
+  const InclusionLocatio

[PATCH] D34269: [clangd] Add "Go to Declaration" functionality

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle-ericsson updated this revision to Diff 103405.
malaperle-ericsson marked 12 inline comments as done.
malaperle-ericsson added a comment.

Address comments


https://reviews.llvm.org/D34269

Files:
  clangd/CMakeLists.txt
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/definitions.test
  test/clangd/formatting.test

Index: test/clangd/formatting.test
===
--- test/clangd/formatting.test
+++ test/clangd/formatting.test
@@ -4,14 +4,15 @@
 Content-Length: 125
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 424
+# CHECK: Content-Length: 462
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
-# CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">"]}
+# CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">"]},
+# CHECK:   "definitionProvider": true
 # CHECK: }}}
 #
 Content-Length: 193
Index: test/clangd/definitions.test
===
--- /dev/null
+++ test/clangd/definitions.test
@@ -0,0 +1,168 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 148
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":2,"character":0}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5}}}]}
+
+Content-Length: 148
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":2,"character":1}}}
+# Go to local variable, end of token
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5}}}]}
+
+Content-Length: 214
+
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///main.cpp","version":2},"contentChanges":[{"text":"struct Foo {\nint x;\n};\nint main() {\n  Foo bar = { x : 1 };\n}\n"}]}}
+
+Content-Length: 149
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":14}}}
+# Go to field, GNU old-style field designator 
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5}}}]}
+
+Content-Length: 215
+
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///main.cpp","version":3},"contentChanges":[{"text":"struct Foo {\nint x;\n};\nint main() {\n  Foo baz = { .x = 2 };\n}\n"}]}}
+
+Content-Length: 149
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":15}}}
+# Go to field, field designator 
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5}}}]}
+
+Content-Length: 187
+
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///main.cpp","version":4},"contentChanges":[{"text":"int main() {\n   main();\n   return 0;\n}"}]}}
+
+Content-Length: 148
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":3}}}
+# Go to function declaration, function call 
+# CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1}}}]}
+
+Content-Length: 208
+
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///main.cpp","version":5},"contentChanges":[{"text":"struct Foo {\n};\nint main() {\n   Foo bar;\n   return 0;\n}\n"}]}}
+
+Content-Length: 148
+
+{"jsonrpc":"2.0","i

[PATCH] D34269: [clangd] Add "Go to Declaration" functionality

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle-ericsson added inline comments.



Comment at: clangd/ClangdUnit.cpp:303
+  // fo|o -> foo|  good!
+  if (InputLocation == SourceLocationBeg && Pos.character > 0) {
+SourceLocation PeekBeforeLocation = Unit->getLocation(FE, Pos.line + 1,

ilya-biryukov wrote:
> Minor: Maybe invert condition to reduce nesting, i.e. if (InputLocation != 
> SourceLocationBeg || Pos.character  == 0) return SourceLocationBeg;
> 
> PS. I'm perfectly fine if this comment is ignored, it's just a matter of 
> preference.
I like it better with the return



Comment at: clangd/ClangdUnit.cpp:306
+Pos.character);
+SourceLocation PeekBeforeLocationEnd = Lexer::getLocForEndOfToken(
+PeekBeforeLocation, 0, SourceMgr, Unit->getASTContext().getLangOpts());

ilya-biryukov wrote:
> Just wondering: is there a way to not do the lexing multiple times?
I was able to simplify this a bit. There's on only one call to getRawToken and 
one to GetBeginningOfToken.



Comment at: clangd/DeclarationLocationsFinder.cpp:48
+  // This can happen when nodes in the AST are visited twice.
+  if (std::none_of(DeclarationLocations.begin(), DeclarationLocations.end(),
+  [&L](const Location& Loc) {

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Is it possible for DeclarationLocation to become large, so that quadratic 
> > behavior is observed?
> > 
> > If not, maybe add an assert that it's smaller than some threshold?
> > If yes, maybe use std::set instead of std::vector or use vector and later 
> > std::sort and std::unique in the end?
> Maybe use std::find instead of std::none_of?
> ```
> std::find(DeclarationLocations.begin(), DeclarationLocations.end(), L) == 
> DeclarationLocations.end()
> ```
I went with std::sort/std::unique. I don't think this will ever be large but I 
don't think it would be good to have an arbitrary limit either. I think keeping 
the vector and cleaning it later is nice and simple.



Comment at: clangd/DeclarationLocationsFinder.cpp:59
+  Token Result;
+  if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(),
+  Unit.getASTContext().getLangOpts(), false)) {

ilya-biryukov wrote:
> Could we implement ` handleMacroOccurence` instead?
> I suspect current code won't work if macro is undef'ed/redefined later:
> 
> ```
> #define FOO 123
> 
> int b = FO|O;
> 
> #define FOO 125
> // or
> // #undef FOO
> ```
> 
> Maybe also add this to tests?
You're right! It didn't work properly in this case. I added a few tests.
For handleMacroOccurence, it actually isn't even called so we'd have to improve 
the clangIndex code to do this. I think this is a good first step though.



Comment at: clangd/Protocol.cpp:57
 std::string URI::unparse(const URI &U) {
-  return U.uri;
+  return "\"" + U.uri + "\"";
 }

ilya-biryukov wrote:
> Why this didn't require any changes to other code? This method hasn't been 
> used before?
No it wasn't used before.



Comment at: test/clangd/definitions.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.

ilya-biryukov wrote:
> Could we also add more readable tests specifically for that?
> I propose to have a tool that could read files like:
> ```
> int aaa;
> int b = a/*{l1}*/aa;
> int c = /*{l2}*/b;
> ```
> 
> And have it output the resulting goto results:
> ```
> l1 -> {main.cpp:0:4} int |aaa;
> l2 -> {main.cpp:1:4} int |b;
> ```
> And we could also have a tool that prints expected clangd input/output based 
> on that to check that action actually works.
> It's not directly relevant to this patch, though. Just random thoughts of 
> what we should do for easier testing.
I think it's a good idea! Although I wonder if it's a bit too much work for 
something that very specific to "textDocument/definition". I fully agree that 
the tests need to be more readable and it would be worth a try!


https://reviews.llvm.org/D34269



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

[PATCH] D34269: [clangd] Add "Go to Declaration" functionality

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle-ericsson added inline comments.



Comment at: clangd/ClangdUnit.cpp:276
+class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::unique_ptr> DeclarationLocations;
+  const SourceLocation &SearchedLocation;

ilya-biryukov wrote:
> Why do we need to use `unique_ptr>` here and in other places 
> instead of `vector`? 
It was to implement "takeLocations". Calling it claims ownship of the vector. 
Did you have something else in mind when you suggested to implement 
takeLocations with a std::move? Disclaimer: this c++11 stuff is all new to me 
so I wouldn't be surprised if I'm doing it in a terrible way.


https://reviews.llvm.org/D34269



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

[PATCH] D34269: [clangd] Add "Go to Declaration" functionality

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle-ericsson added inline comments.



Comment at: clangd/ClangdUnit.cpp:276
+class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::unique_ptr> DeclarationLocations;
+  const SourceLocation &SearchedLocation;

malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > Why do we need to use `unique_ptr>` here and in other 
> > places instead of `vector`? 
> It was to implement "takeLocations". Calling it claims ownship of the vector. 
> Did you have something else in mind when you suggested to implement 
> takeLocations with a std::move? Disclaimer: this c++11 stuff is all new to me 
> so I wouldn't be surprised if I'm doing it in a terrible way.
I guess I can make takeLocations return a vector&& and the other places will 
return a normal vector RVO will take care of not making copies?


https://reviews.llvm.org/D34269



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

[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: mgrang.

Needs to be rebased.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894



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

[PATCH] D39375: [clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:263
   const MacroDirective *MD);
+  /// Adds list of Preprocessor callbacks so we can also process information
+  /// about includes that are outside of a preamble i.e in the middle of a file

It doesn't add, it creates.It's also not a "list". Is it true that it will 
process includes outside the preamble? I would think building the preamble 
stopped at the end of the preamble.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:354
 
+  if (Callbacks.createPPCallbacks())
+
Clang->getPreprocessor().addPPCallbacks(std::move(Callbacks.createPPCallbacks()));

extract this to a local variable


Repository:
  rC Clang

https://reviews.llvm.org/D39375



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

[PATCH] D40548: [clangd] Symbol index interfaces and an in-memory index implementation.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D40548#947081, @ioeric wrote:

> Hi Marc, the patch is not ready for review yet. I am still cleaning up the 
> prototype and will let you know when it's ready for review.


I guess it was ready to review since it was submitted? ;)




Comment at: clangd/index/Index.h:134
+  virtual bool
+  fuzzyFind(Context &Ctx, const FuzzyFindRequest &Req,
+std::function Callback) const = 0;

I think a more generic std::function would be useful, similar to the 
indexstore's filter
```
bool searchSymbols(llvm::function_ref 
filter,
 llvm::function_ref receiver)
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D40548: [clangd] Symbol index interfaces and an in-memory index implementation.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/index/Index.h:134
+  virtual bool
+  fuzzyFind(Context &Ctx, const FuzzyFindRequest &Req,
+std::function Callback) const = 0;

ioeric wrote:
> malaperle wrote:
> > I think a more generic std::function would be useful, similar to the 
> > indexstore's filter
> > ```
> > bool searchSymbols(llvm::function_ref 
> > filter,
> >  llvm::function_ref receiver)
> > ```
> Do you have an use case in mind which requires different filtering? This 
> could probably be a separate interface. 
> 
> I think the behavior of fuzzy finding is well-defined, and how filtering is 
> done is an implementation detail. User-specified filter might make 
> implementation less flexible, especially for large indexes that are not 
> managed in memory. 
For example
- Searching by USR
- By non-qualified name (useful also for Open Workspace Symbol)
- Most likely other filters, hmm, by Symbol kind, language?
- No filter, i.e. retrieve *all* symbols. Useful mainly for development and to 
get index statistics

There could also be a boolean in the callback return value (or the filter 
callback) to limit the number of returned values.
I haven't tried locally but it looks like it would be pretty doable to change 
the FuzzyFindRequest into a std::function.
A big disadvantage of having just one method though is that is would be 
difficult for an implementation to optimize for a specific type of query. For 
example, if you search by non-qualified name, an implementation could use a 
mapping of non-qualified-name-to-USR but it would be difficult to know what to 
do given only the std::function/filter passed by argument.
In that sense, perhaps it is better to have multiple methods for each use 
cases. Or perhaps some enum for each kind of query. What do you think?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.



Comment at: clangd/ClangdServer.cpp:454
+
+IncludeReferenceMap IRM = std::move(AST->takeIRM());
+Result = clangd::findDefinitions(*AST, Pos, Logger, 
IRM.IncludeLocationMap);

IncludeReferenceMap & here? See other comment in takeIRM



Comment at: clangd/ClangdUnit.cpp:34
 #include 
+#include 
 

remove



Comment at: clangd/ClangdUnit.cpp:77
 
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+void fillRangeVector(
+const SourceManager &SM,

remove



Comment at: clangd/ClangdUnit.cpp:96
+
+void findPreambleIncludes(
+const SourceManager &SM,

remove



Comment at: clangd/ClangdUnit.cpp:118
+  CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+  : SourceMgr(SourceMgr), IRM(IRM) {}
+

These need to be swapped to be in the same order that they are declared below.



Comment at: clangd/ClangdUnit.cpp:120
+
+  IncludeReferenceMap takeIRM() {
+fillRangeVector(*SourceMgr, IRM.DataVector, IRM.RangeVector);

I don't think we need this if we pass the map by reference (and store it as a 
reference, see other comment)



Comment at: clangd/ClangdUnit.cpp:127
+
+  IncludeReferenceMap getIRM() { return IRM; };
+

Remove (see previous comment)



Comment at: clangd/ClangdUnit.cpp:155
+  }
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,

I tried to simply the methods introduced here:
```
  void AfterExecute(CompilerInstance &CI) override {
SourceMgr = &CI.getSourceManager();
for (auto InclusionLoc : TempPreambleIncludeLocations)
  addIncludeLocation(InclusionLoc);
  }

  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
  StringRef FileName, bool IsAngled,
  CharSourceRange FilenameRange, const FileEntry *File,
  StringRef SearchPath, StringRef RelativePath,
  const Module *Imported) override {
auto SR = FilenameRange.getAsRange();
if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
  return;

if (SourceMgr) {
  addIncludeLocation({SR, File->tryGetRealPathName()});
} else {
  // When we are processing the inclusion directives inside the preamble,
  // we don't have access to a SourceManager, so we cannot convert
  // SourceRange to Range. This is done instead in AfterExecute when a
  // SourceManager becomes available.
  TempPreambleIncludeLocations.push_back({SR, File->tryGetRealPathName()});
}
  }

  void addIncludeLocation(std::pair InclusionLoc) {
// Only inclusion directives in the main file make sense. The user cannot
// select directives not in the main file.
if (SourceMgr->getMainFileID() == 
SourceMgr->getFileID(InclusionLoc.first.getBegin()))
  IRM.insert({getRange(InclusionLoc.first), InclusionLoc.second});
  }

  Range getRange(SourceRange SR) {
Position Begin;
Begin.line = SourceMgr->getSpellingLineNumber(SR.getBegin());
Begin.character = SourceMgr->getSpellingColumnNumber(SR.getBegin());
Position End;
End.line = SourceMgr->getSpellingLineNumber(SR.getEnd());
End.character = SourceMgr->getSpellingColumnNumber(SR.getEnd());
return {Begin, End};
  }
```



Comment at: clangd/ClangdUnit.cpp:177
+  Range R = {Begin, End};
+  if (File && !File->tryGetRealPathName().empty())
+IRM.IncludeLocationMap.insert({R, File->tryGetRealPathName()});

I think we need a "is main file" check here. In case this is used on a AST with 
no preamble.



Comment at: clangd/ClangdUnit.cpp:209
   std::vector TopLevelDeclIDs;
+  IncludeReferenceMap IRM;
+  SourceManager *SourceMgr;

 IncludeReferenceMap &IRM;

That way, we can use the same map and pass it multiple times to different 
"collectors".



Comment at: clangd/ClangdUnit.cpp:266
 
+class EmptyDiagsConsumer : public DiagnosticConsumer {
+public:

I don't think this change was brought back intentionally?



Comment at: clangd/ClangdUnit.cpp:288
  IntrusiveRefCntPtr VFS,
- clangd::Logger &Logger) {
+ clangd::Logger &Logger, IncludeReferenceMap IRM) {
 

IncludeReferenceMap &IRM



Comment at: clangd/ClangdUnit.cpp:314
+
+  
Clang->getPreprocessor().addPPCallbacks(std::move(SerializedDeclsCollector.createPPCallbacks()));
+

unnecessary std::move



Comment at: clangd/ClangdUnit.cpp:325
+   std::move(ParsedDecls), std::move(ASTDi

[PATCH] D39375: [clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle accepted this revision.
malaperle added a comment.

Looks good to me. But this is not really my area :)


Repository:
  rC Clang

https://reviews.llvm.org/D39375



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

[PATCH] D40548: [clangd] Symbol index interfaces and an in-memory index implementation.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/index/Index.h:134
+  virtual bool
+  fuzzyFind(Context &Ctx, const FuzzyFindRequest &Req,
+std::function Callback) const = 0;

sammccall wrote:
> ioeric wrote:
> > malaperle wrote:
> > > ioeric wrote:
> > > > malaperle wrote:
> > > > > I think a more generic std::function would be useful, similar to the 
> > > > > indexstore's filter
> > > > > ```
> > > > > bool searchSymbols(llvm::function_ref > > > > &stop)> filter,
> > > > >  llvm::function_ref 
> > > > > receiver)
> > > > > ```
> > > > Do you have an use case in mind which requires different filtering? 
> > > > This could probably be a separate interface. 
> > > > 
> > > > I think the behavior of fuzzy finding is well-defined, and how 
> > > > filtering is done is an implementation detail. User-specified filter 
> > > > might make implementation less flexible, especially for large indexes 
> > > > that are not managed in memory. 
> > > For example
> > > - Searching by USR
> > > - By non-qualified name (useful also for Open Workspace Symbol)
> > > - Most likely other filters, hmm, by Symbol kind, language?
> > > - No filter, i.e. retrieve *all* symbols. Useful mainly for development 
> > > and to get index statistics
> > > 
> > > There could also be a boolean in the callback return value (or the filter 
> > > callback) to limit the number of returned values.
> > > I haven't tried locally but it looks like it would be pretty doable to 
> > > change the FuzzyFindRequest into a std::function.
> > > A big disadvantage of having just one method though is that is would be 
> > > difficult for an implementation to optimize for a specific type of query. 
> > > For example, if you search by non-qualified name, an implementation could 
> > > use a mapping of non-qualified-name-to-USR but it would be difficult to 
> > > know what to do given only the std::function/filter passed by argument.
> > > In that sense, perhaps it is better to have multiple methods for each use 
> > > cases. Or perhaps some enum for each kind of query. What do you think?
> > Adding new interfaces is an option. It should also be easy to extend the 
> > existing interface to cover more use cases.
> > 
> > > Searching by USR
> > We definitely want an interface for this. I can see this being useful for 
> > Goto-definition and other features.
> > > By non-qualified name (useful also for Open Workspace Symbol)
> > I think `fuzzyFind` can support this, with fuzzy matching optionally turned 
> > off if preferred.
> > > Most likely other filters, hmm, by Symbol kind, language?
> > Similarly, this could be achieved by extending the find request to include 
> > more filters e.g. symbol kind. 
> > > No filter, i.e. retrieve *all* symbols. Useful mainly for development and 
> > > to get index statistics
> > This can be done today by setting an empty fuzzy filter.
> > 
> > 
> As Eric said, we should be able to add more options here.
> But why are we reluctant to add an arbitrary std::function, which is more 
> elegant and flexible?
> 
>  - it essentially forbids remote indexes. Because functions aren't 
> serializable, the only possible implementation is to send all the symbols to 
> clangd. For our main codebase, this is would be ~1G for just qnames, and a 
> lot more for full symbols.
>  - A slightly more subtle disadvantage is because functions aren't 
> serializable, it's harder to debug them - you can't just log the query.
>  - filtering functions are easy to implement (so can be distributed over 
> callsites), but scoring functions are hard (so should be more centralized). 
> They tend to operate on the same data. It's not obvious how to achieve this 
> with a filter API. 
Thanks a lot, that makes sense.

I do think it would be good, perhaps for other kinds of queries, that the query 
could be stopped halfway by having the Callback return a bool.
```
std::function Callback) const
```
to
```
std::function Callback) const
```

But this can be revisited once we have other options.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D41306: [clangd] Update documentation page with new features, instructions

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, ilya-biryukov, klimek.

- Some features were implemented so mark them as such.
- Add installation instructions (LLVM debian packages)

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41306

Files:
  docs/clangd.rst


Index: docs/clangd.rst
===
--- docs/clangd.rst
+++ docs/clangd.rst
@@ -23,8 +23,20 @@
 used in order to test :program:`Clangd` but more clients are likely to make
 use of :program:`Clangd` in the future as it matures and becomes a production
 quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `building Clangd`_, then open Visual
-Studio Code in the clangd-vscode folder and launch the extension.
+with Visual Studio Code, you can start by `installing Clangd`_ or
+`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder 
and
+launch the extension.
+
+Installing Clangd
+==
+
+Packages are available for debian-based distributions, see the `LLVM packages
+page `_. :program:`Clangd` is included in the
+`clang-tools` package.
+However, it is a good idea to check your distribution's packaging system first
+as it might already be available.
+
+Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
 
 Building Clangd
 ==
@@ -41,7 +53,8 @@
 not they are already implemented in :program:`Clangd` and specified in the
 Language Server Protocol. Note that for some of the features, it is not clear
 whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd`.
+features might be eventually developed outside :program:`Clangd` or as an
+extension to the protocol.
 
 +-++--+
 | C/C++ Editor feature|  LSP   |  Clangd  |
@@ -56,18 +69,22 @@
 +-++--+
 | Go to Definition| Yes|   Yes|
 +-++--+
-| Source hover| Yes|   No |
+| Signature Help  | Yes|   Yes|
 +-++--+
-| Signature Help  | Yes|   No |
+| Document Highlights | Yes|   Yes|
 +-++--+
-| Find References | Yes|   No |
+| Rename  | Yes|   Yes|
 +-++--+
-| Document Highlights | Yes|   No |
+| Source hover| Yes|   No |
 +-++--+
-| Rename  | Yes|   No |
+| Find References | Yes|   No |
 +-++--+
 | Code Lens   | Yes|   No |
 +-++--+
+| Document Symbols| Yes|   No |
++-++--+
+| Workspace Symbols   | Yes|   No |
++-++--+
 | Syntax and Semantic Coloring| No |   No |
 +-++--+
 | Code folding| No |   No |


Index: docs/clangd.rst
===
--- docs/clangd.rst
+++ docs/clangd.rst
@@ -23,8 +23,20 @@
 used in order to test :program:`Clangd` but more clients are likely to make
 use of :program:`Clangd` in the future as it matures and becomes a production
 quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `building Clangd`_, then open Visual
-Studio Code in the clangd-vscode folder and launch the extension.
+with Visual Studio Code, you can start by `installing Clangd`_ or
+`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
+launch the extension.
+
+Installing Clangd
+==
+
+Packages are available for debian-based distributions, see the `LLVM packages
+page `_. :program:`Clangd` is included in the
+`clang-tools` package.
+However, it is a good idea to check your distribution's packaging system first
+as it might already be available.
+
+Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
 
 Building Clangd
 ==
@@ -41,7 +53,8 @@
 not t

[PATCH] D41306: [clangd] Update documentation page with new features, instructions

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL320988: [clangd] Update documentation page with new 
features, instructions (authored by malaperle, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D41306

Files:
  clang-tools-extra/trunk/docs/clangd.rst


Index: clang-tools-extra/trunk/docs/clangd.rst
===
--- clang-tools-extra/trunk/docs/clangd.rst
+++ clang-tools-extra/trunk/docs/clangd.rst
@@ -23,8 +23,20 @@
 used in order to test :program:`Clangd` but more clients are likely to make
 use of :program:`Clangd` in the future as it matures and becomes a production
 quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `building Clangd`_, then open Visual
-Studio Code in the clangd-vscode folder and launch the extension.
+with Visual Studio Code, you can start by `installing Clangd`_ or
+`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder 
and
+launch the extension.
+
+Installing Clangd
+==
+
+Packages are available for debian-based distributions, see the `LLVM packages
+page `_. :program:`Clangd` is included in the
+`clang-tools` package.
+However, it is a good idea to check your distribution's packaging system first
+as it might already be available.
+
+Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
 
 Building Clangd
 ==
@@ -41,7 +53,8 @@
 not they are already implemented in :program:`Clangd` and specified in the
 Language Server Protocol. Note that for some of the features, it is not clear
 whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd`.
+features might be eventually developed outside :program:`Clangd` or as an
+extension to the protocol.
 
 +-++--+
 | C/C++ Editor feature|  LSP   |  Clangd  |
@@ -56,18 +69,22 @@
 +-++--+
 | Go to Definition| Yes|   Yes|
 +-++--+
-| Source hover| Yes|   No |
+| Signature Help  | Yes|   Yes|
 +-++--+
-| Signature Help  | Yes|   No |
+| Document Highlights | Yes|   Yes|
 +-++--+
-| Find References | Yes|   No |
+| Rename  | Yes|   Yes|
 +-++--+
-| Document Highlights | Yes|   No |
+| Source hover| Yes|   No |
 +-++--+
-| Rename  | Yes|   No |
+| Find References | Yes|   No |
 +-++--+
 | Code Lens   | Yes|   No |
 +-++--+
+| Document Symbols| Yes|   No |
++-++--+
+| Workspace Symbols   | Yes|   No |
++-++--+
 | Syntax and Semantic Coloring| No |   No |
 +-++--+
 | Code folding| No |   No |


Index: clang-tools-extra/trunk/docs/clangd.rst
===
--- clang-tools-extra/trunk/docs/clangd.rst
+++ clang-tools-extra/trunk/docs/clangd.rst
@@ -23,8 +23,20 @@
 used in order to test :program:`Clangd` but more clients are likely to make
 use of :program:`Clangd` in the future as it matures and becomes a production
 quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `building Clangd`_, then open Visual
-Studio Code in the clangd-vscode folder and launch the extension.
+with Visual Studio Code, you can start by `installing Clangd`_ or
+`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
+launch the extension.
+
+Installing Clangd
+==
+
+Packages are available for debian-based distributions, see the `LLVM packages
+page `_. :program:`Clangd` is included in the
+`clang-tools` package.
+However, it is a good idea to check your distribution's packaging system first
+as it might already be available.
+
+Otherwise, you can

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

@ilya-biryukov Hi! I'll be updating William's patches that were in progress. I 
just have a few comments/question before I send a new update. (I also don't 
know if I can update this diff or I have to create a new diff on Phabricator... 
I guess we'll see!!).




Comment at: clangd/ClangdUnit.cpp:113
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+  : IRM(IRM) {}

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Let's create a new empty map inside this class and have a 
> > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and 
> > `takeTopLevelDeclIDs`)
> This comment is not addressed yet, but marked as done.
As mentioned below, the idea was to have a single map being appended to, 
without having to merge two separate maps. However, I can change the code so 
that two separate maps are built and merged if you prefer.

But I'm not so clear if that's what you'd prefer:

> You copy the map for preamble and then append to it in 
> CppFilePreambleCallbacks? That also LG, we should not have many references 
> there anyway.

It's not meant to have any copy. The idea was to create a single 
IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble 
and non-preamble include references and std::move it around for later use 
(stored in ParsedAST).



Comment at: clangd/ClangdUnit.cpp:281
+std::shared_ptr PCHs,
+IntrusiveRefCntPtr VFS, IncludeReferenceMap IRM) {
 

ilya-biryukov wrote:
> Don't we already store the map we need in `PreambleData`? Why do we need an 
> extra `IRM` parameter?
Since the map will be now stored in ParsedAST and the instance doesn't exist 
yet, we need to keep the IRM parameter. I noticed that it wasn't being 
std::move'd though.



Comment at: clangd/XRefs.cpp:185
+
+  if ((unsigned)R.start.line ==
+  SourceMgr.getSpellingLineNumber(

ilya-biryukov wrote:
> why do we need to convert to unsigned? To slience compiler warnings?
Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber 
returns unsigned. I'll extract another var for the line number and cast both to 
int instead to have less casts and make the condition smaller.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/ClangdUnit.cpp:113
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+  : IRM(IRM) {}

ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > Let's create a new empty map inside this class and have a 
> > > > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and 
> > > > `takeTopLevelDeclIDs`)
> > > This comment is not addressed yet, but marked as done.
> > As mentioned below, the idea was to have a single map being appended to, 
> > without having to merge two separate maps. However, I can change the code 
> > so that two separate maps are built and merged if you prefer.
> > 
> > But I'm not so clear if that's what you'd prefer:
> > 
> > > You copy the map for preamble and then append to it in 
> > > CppFilePreambleCallbacks? That also LG, we should not have many 
> > > references there anyway.
> > 
> > It's not meant to have any copy. The idea was to create a single 
> > IncludeReferenceMap in CppFile::deferRebuild, populate it with both 
> > preamble and non-preamble include references and std::move it around for 
> > later use (stored in ParsedAST).
> We can't have a single map because AST is rebuilt more often than the 
> Preamble, so we have two options:
> - Store a map for the preamble separately, copy it when we need to rebuild 
> the AST and append references from the AST to the new instance of the map.
> - Store two maps: one contains references only from the Preamble, the other 
> one from the AST.
> 
> I think both are fine, since the copy of the map will be cheap anyway, as we 
> only store a list of includes inside the main file.
> We can't have a single map because AST is rebuilt more often than the 
> Preamble, so we have two options:

Doh! Indeed.

OK so I added the map in PreambleData, this one will say every reparse unless 
the preamble changes. When the AST is built/rebuilt, the map is copied (or 
empty) and includes from the AST are appended to it then stored in ParsedAST 
(option #1?).



Comment at: clangd/XRefs.cpp:185
+
+  if ((unsigned)R.start.line ==
+  SourceMgr.getSpellingLineNumber(

ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > why do we need to convert to unsigned? To slience compiler warnings?
> > Yes, "line" from the protocol is signed, whereas 
> > getSpellingColumn/lineNumber returns unsigned. I'll extract another var for 
> > the line number and cast both to int instead to have less casts and make 
> > the condition smaller.
> Can we maybe convert to `clangd::Position` using the helper methods first and 
> do the comparison of two `clangd::Position`s?
> Comparing between `clangd::Position` and clang's line/column numbers is a 
> common source of off-by-one errors in clangd.
offsetToPosition (if that's what you mean) uses a StringRef of the code, which 
is not handy in this case. I'll add another one "sourceLocToPosition" to 
convert a SourceLocation to a Position. WDYT? It can be used in a few other 
places too.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 129292.
malaperle added a comment.

Store map in PremableData and copy it on reparse.
Convert SourceLoc to Position when comparing with include Positions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -751,6 +751,74 @@
   EXPECT_FALSE(PathResult.hasValue());
 }
 
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents = R"cpp(
+  #include "foo.h"
+  #include "invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #include "foo.h"
+  )cpp";
+  FS.Files[FooCpp] = SourceContents;
+  auto FooH = getVirtualTestFilePath("foo.h");
+  const auto HeaderContents = "int a;";
+
+  FS.Files[FooCpp] = SourceContents;
+  FS.Files[FooH] = HeaderContents;
+
+  Server.addDocument(Context::empty(), FooH, HeaderContents);
+  Server.addDocument(Context::empty(), FooCpp, SourceContents);
+
+  Position P = Position{1, 11};
+
+  auto ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P);
+  ASSERT_TRUE(!!ExpectedLocations);
+  std::vector Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  std::string s("file:///");
+  std::string check = Locations[0].uri.uri;
+  check = check.erase(0, s.size() - 1);
+  check = check.substr(0, check.size());
+  ASSERT_EQ(check, FooH);
+  ASSERT_EQ(Locations[0].range.start.line, 0);
+  ASSERT_EQ(Locations[0].range.start.character, 0);
+  ASSERT_EQ(Locations[0].range.end.line, 0);
+  ASSERT_EQ(Locations[0].range.end.character, 0);
+
+  // Test include in preamble
+  Position P2 = Position{1, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P2);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+
+  // Test invalid include
+  Position P3 = Position{2, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P3);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  // Test include outside of Preamble
+  Position P4 = Position{6, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P4);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+}
+
 TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -7,6 +7,7 @@
 //
 //===-===//
 #include "XRefs.h"
+#include "SourceCode.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 namespace clang {
@@ -103,12 +104,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -142,6 +139,21 @@
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
  DeclMacrosFinder, IndexOpts);
 
+  /// Process targets for paths inside #include directive.
+  std::vector IncludeTargets;
+  for (auto &IncludeLoc : AST.getIRM()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+  IncludeTargets.push_back(Location{URI::fromFile(IncludeLoc.second),
+Range{Position{0, 0}, Position{0, 0}}});
+  return IncludeTargets;
+}
+  }
+
   std::vector Decls = DeclMacrosFinder->takeDecls();
   std::vector MacroInfos =
   DeclMacrosFinder->take

[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 129293.
malaperle added a comment.

Revert an unintentional white space change.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -751,6 +751,74 @@
   EXPECT_FALSE(PathResult.hasValue());
 }
 
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+  /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents = R"cpp(
+  #include "foo.h"
+  #include "invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #include "foo.h"
+  )cpp";
+  FS.Files[FooCpp] = SourceContents;
+  auto FooH = getVirtualTestFilePath("foo.h");
+  const auto HeaderContents = "int a;";
+
+  FS.Files[FooCpp] = SourceContents;
+  FS.Files[FooH] = HeaderContents;
+
+  Server.addDocument(Context::empty(), FooH, HeaderContents);
+  Server.addDocument(Context::empty(), FooCpp, SourceContents);
+
+  Position P = Position{1, 11};
+
+  auto ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P);
+  ASSERT_TRUE(!!ExpectedLocations);
+  std::vector Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  std::string s("file:///");
+  std::string check = Locations[0].uri.uri;
+  check = check.erase(0, s.size() - 1);
+  check = check.substr(0, check.size());
+  ASSERT_EQ(check, FooH);
+  ASSERT_EQ(Locations[0].range.start.line, 0);
+  ASSERT_EQ(Locations[0].range.start.character, 0);
+  ASSERT_EQ(Locations[0].range.end.line, 0);
+  ASSERT_EQ(Locations[0].range.end.character, 0);
+
+  // Test include in preamble
+  Position P2 = Position{1, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P2);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+
+  // Test invalid include
+  Position P3 = Position{2, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P3);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  // Test include outside of Preamble
+  Position P4 = Position{6, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P4);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+}
+
 TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -7,6 +7,7 @@
 //
 //===-===//
 #include "XRefs.h"
+#include "SourceCode.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 namespace clang {
@@ -103,12 +104,8 @@
 return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
  SourceMgr, LangOpts);
-  Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-  Position End;
-  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
@@ -142,6 +139,21 @@
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
  DeclMacrosFinder, IndexOpts);
 
+  /// Process targets for paths inside #include directive.
+  std::vector IncludeTargets;
+  for (auto &IncludeLoc : AST.getIRM()) {
+Range R = IncludeLoc.first;
+const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+Pos.character <= R.end.character) {
+  IncludeTargets.push_back(Location{URI::fromFile(IncludeLoc.second),
+Range{Position{0, 0}, Position{0, 0}}});
+  return IncludeTargets;
+}
+  }
+
   std::vector Decls = DeclMacrosFinder->takeDecls();
   std::vector MacroInfos =
   DeclMacrosFinder->takeMacroInfos();
@@ -217,13 +229,8 @@
   DocumentHighlight getDocumentHighligh

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#949185, @malaperle wrote:

> In https://reviews.llvm.org/D39050#948500, @akyrtzi wrote:
>
> > @malaperle, to clarify we are not suggesting that you write your own 
> > parser, the suggestion is to use clang in 'fast-scan' mode to get the 
> > structure of the declarations of a single file, see 
> > `CXTranslationUnit_SingleFileParse` (along with enabling skipping of 
> > bodies). We have found clang is super fast when you only try to get the 
> > structure of a file like this.
>
>
> Thank you, that sounds very useful. I will try that and get some measurements.
>
> > We can make convenient APIs to provide the syntactic structure of 
> > declarations based on their location.
>
> Perhaps just for the end-loc since it's pretty much guaranteed to be needed 
> by everyone. But if it's very straightforward, perhaps that's not needed. 
> I'll try and see.
>
> > But let's say we added the end-loc, is it enough ? If you want to implement 
> > the 'peek the definition' like Eclipse, then it is not enough, you also 
> > need to figure out if there are documentation comments associated with the 
> > declaration and also show those. Also what if you want to highlight the 
> > type signature of a function, then just storing the location of the closing 
> > brace of its body is not enough. There can be any arbitrary things you may 
> > want to get from the structure of the declaration (e.g. the parameter 
> > ranges), but we could provide an API to gather any syntactic structure info 
> > you may want.
>
> That's a very good point. I guess in the back of my mind, I have the worry 
> that one cannot extend what is stored, either for a different performance 
> trade-off or for additional things. The fact that both clang and clangd have 
> to agree on the format so that index-while-building can be used seems to make 
> it inherently not possible to extend. But perhaps it's better to not 
> overthink this for now.


I did a bit more of experimenting. For the end-loc, I changed my prototype so 
that the end-loc is not stored in the index but rather computed "on the fly" 
using SourceManager and Lexer only. For my little benchmark, I used the 
LLVM/Clang/Clangd code base which I queried for all references of "std" (the 
namespace) which is around 46K references in the index.

With end-loc in index: 3.45s on average(20 samples)
With end-loc computed on the fly: 11.33s on average(20 samples)
I also tried with Xcode but without too much success: it took about 30 secs to 
reach 45K results and then carried on for a long time and hung (although I 
didn't try to leave it for hours to see if it finished).

From my perspective, it seems that the extra time is quite substantial and it 
doesn't seem worth to save an integer per occurrence in this case.

For computing the start/end-loc of function bodies, I tried the 
SingleFileParseMode and SkipFunctionBodies separately ( as a start). The source 
I use this on looks like this:

  #include "MyClass.h"
  
  MyClass::MyClass() {
  }
  
  void MyClass::doOperation() {
  }

With SingleFileParseMode, I get several errors:

> MyClass.cpp:5:1: error: use of undeclared identifier 'MyClass'
>  MyClass.cpp:8:6: error: use of undeclared identifier 'MyClass'

Then I cannot obtain any Decl* at the position of doOperation. With 
SingleFileParseMode, I'm also a bit weary that not processing headers will 
result in many inaccuracies. From our perspective, we are more wiling to 
sacrifice disk space in order to have more accuracy and speed. For comparison, 
the index I worked with containing all end-loc for occurrences and also 
function start/end is 201M for LLVM/Clang/Clangd which is small to us.

With SkipFunctionBodies alone, I can get the Decl* but 
FunctionDecl::getSourceRange() doesn't include the body, rather, it stops after 
the arguments.
It would be very nice if we could do this cheaply but it doesn't seem possible 
with those two flags alone. What did you have in mind for implementing an "API 
to gather any syntactic structure info" ?


https://reviews.llvm.org/D39050



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

[PATCH] D44213: [clangd] Remove unused field in HandlerRegisterer

[Marc-Andre Laperle via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44213



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

[PATCH] D44247: [clangd] Use identifier range as the definition range.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

I was going to change the symbol index to do the opposite :) The range of 
definitions including the bodies of functions, etc is used in a "peek 
definition" feature by several LSP clients. So for example in VSCode, you can 
hold Ctrl and hover on a function call and see its definition in a popup. There 
was some discussion about this in https://reviews.llvm.org/D35894


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44247



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

[PATCH] D44251: [clangd] Early return for #include goto definition.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44251#1031370, @ilya-biryukov wrote:

> LGTM.
>  I think this was part of initial changed and I asked to make it this way, 
> but I don't remember why.


I think it was just to have a single place that returns. I think it's OK to 
early return in this case, it's a good optimization.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44251



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

[PATCH] D44247: [clangd] Use identifier range as the definition range.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44247#1031429, @sammccall wrote:

> In https://reviews.llvm.org/D44247#1031366, @sammccall wrote:
>
> > In https://reviews.llvm.org/D44247#1031345, @malaperle wrote:
> >
> > > I was going to change the symbol index to do the opposite :) The range of 
> > > definitions including the bodies of functions, etc is used in a "peek 
> > > definition" feature by several LSP clients. So for example in VSCode, you 
> > > can hold Ctrl and hover on a function call and see its definition in a 
> > > popup. There was some discussion about this in 
> > > https://reviews.llvm.org/D35894
> >
> >
> > Interesting - that seemed like a more natural interpretation of LSP to me.
> >  Others talked me down, motivated (I think) by nicer behavior of 
> > jump-to-definition... will bring it up again :)
>
>
> Man, this seemed compelling to me, but there's a little bit of wiggle room in 
> the spec (what's the "definition location"), so we looked at the MS language 
> servers...
>  ... and both their TS and C++ implementations return the range of the name 
> only, despite that (IMO) being a weird interpretation of the spec.
>  As for VSCode:
>
> - the "ctrl-to-hover" behavior starts at beginning of the line containing the 
> range, and has a heuristic for when to stop (even if you return the whole 
> definition range, I think). So whole-range is a bit better here (particularly 
> when type/template is on a separate line) but actually still not great.
> - "peek definition" shows the selected range in the middle of a block, with 
> the range highlighted. Having the whole code highlighted actually looks kinda 
> bad :/
> - "go to definition" puts your cursor at the start of the range, and the 
> identifier seems much better here.
>
>   So I *want* to agree, but we'll be fighting the other language servers and 
> editors (and @ioeric, @ilya-biryukov who think we'd be breaking more 
> important workflows)... I think we're actually better off just returning the 
> name.


Good points, thanks for investigating this. I agree that it should return the 
range of the name only so the direction of this patch looks good to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44247



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

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#1021204, @malaperle wrote:

> For computing the start/end-loc of function bodies, I tried the 
> SingleFileParseMode and SkipFunctionBodies separately ( as a start). The 
> source I use this on looks like this:
>
>  


Given the discussion in https://reviews.llvm.org/D44247, I think we can do 
without the start/end-loc of function bodies and try some heuristics 
client-side. We can always revisit this later if necessary.

However, for the end-loc of occurrences, would you be OK with this being added? 
I think it would be a good compromise in terms of performance, simplicity and 
index size.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#1036394, @nathawes wrote:

> @malaperle Just to clarify, what's the particular end-loc we're talking about 
> here? e.g. for a function call, would this be the end of the function's name, 
> or the closing paren? 
>  For the end of the name, couldn't this be derived from the start loc + 
> symbol name length (barring token pastes and escaped new lines in the middle 
> of identifiers, which hopefully aren't too common)?
>  I can see the value for the closing paren though.


I mean the end of the name referencing the symbol, so that it can be 
highlighted properly when using the "find references in workspace" feature. 
There are cases where the name of the symbol itself is not present, for example 
"MyClass o1, o2;" (o1 and o2 reference the constructor), references to 
overloaded operators, etc.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#1037796, @akyrtzi wrote:

> Hey Marc,
>
> > The fact that both clang and clangd have to agree on the format so that 
> > index-while-building can be used seems to make it inherently not possible 
> > to extend
>
> I don't think "not possible to extend" is quite correct, we can make it so 
> that the format allows optional data to be recorded.


That would be good. How would one go about asking Clang to generate this extra 
information? Would a Clang Plugin be suitable for this? I don't know much about 
those but perhaps that could be one way to extent the basic behavior of 
"-index_store_path" in this way?

>>   I changed my prototype so that the end-loc is not stored in the index but 
>> rather computed "on the fly" using SourceManager and Lexer only.
> 
> I assume you used SingleFileParseMode+SkipFunctionBodies for this, right ?

No, sorry the end-locs I meant there is for occurrences. Only the lexer was 
needed to get the end of the token. So for "MyClass o1, o2;" o1 and o2 get 
highlighted as references to the MyClass constructor.

> 
> 
>> For my little benchmark, I used the LLVM/Clang/Clangd code base which I 
>> queried for all references of "std" (the namespace) which is around 46K 
>> references in the index.
> 
> This is an interesting use case, and I can say we have some experience 
> because Xcode has this functionality without requiring the end-loc for every 
> reference.
>  So what it does is that it has a 'name' to look for (say 'foo' for  the 
> variable foo) and if it finds the name in the location then it highlights, 
> otherwise if it doesn't find it (e.g. because it is an implicit reference) 
> then it points to the location but doesn't highlight something.

I think it's useful to highlight something even when the name is not there. For 
example in "MyClass o1, o2;" it feels natural that o1 and o2 would get 
highlighted.

> The same thing happens for operator overloads (the operators get highlighted 
> at the reference location).

It does? I can only seem to do a textual search. For example, if I look at 
"FileId::operator<", if I right-click in the middle of "operator<" and do "Find 
selected symbol in workspace", it seems to start a text based search because 
there are many results that are semantically unrelated.

> For implicit references it's most likely there's nothing to highlight so the 
> end-loc will most likely be empty anyway (or same as start-loc ?) to indicate 
> an empty range.

I think for those cases the end of the token is probably suitable. Can you give 
examples which implicit references you have in mind? Maybe another one (other 
than the constructor mentioned above) could be a function call like 
"passMeAStdString(MyStringRef)", here the "operator std::string" would be 
called and MyStringRef could be highlighted, I think it would make sense to the 
user that is gets called by passing this parameter by seeing the highlight.

> Going back to the topic of what use cases end-loc covers, note that it still 
> seems inadequate for peek-definition functionality. You can't set it to 
> body-end loc (otherwise occurrences highlighting would highlight the whole 
> body which I think is undesirable) and you still need to include doc-comments 
> if they exist.

I think maybe I wasn't clear, I was thinking about two end-locs: end-locs of 
occurrences and end-locs of bodies. The end-loc of occurrences would be used 
for highlight when searching for all occurrences and the end-loc for bodies 
would be used for the peek definition. I think we can disregard end-locs of 
bodies for now.


https://reviews.llvm.org/D39050



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

[PATCH] D39050: Add index-while-building support to Clang

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D39050#1040501, @akyrtzi wrote:

> > That would be good. How would one go about asking Clang to generate this 
> > extra information? Would a Clang Plugin be suitable for this?
>
> That's an interesting idea that we could explore, but I don't have much 
> experience with that mechanism to comment on.
>
> > Only the lexer was needed to get the end of the token
>
> Ok, that's interesting, not sure why Xcode is so fast to highlight, did you 
> reuse same SourceManager/Lexer/buffers for occurrences from same file ? We'd 
> definitely add the end-loc if we cannot come up with a mechanism to highlight 
> fast enough without it.


I don't think Xcode is quite fast, it's about 10 times slower (although I'm not 
sure it really finished) than when I use my branch that has the end-loc. I 
would try end-locs in Xcode if I could, to compare :) So I don't really know 
where the bottleneck is in Xcode. Comparing oranges to oranges, it's 4 times 
slower without end-locs compared to with end-locs on my branch. I does use the 
same SourceManager for the 46K references and I verified that it uses the same 
buffers, etc.
I'll put the numbers here again for readability.

> For my little benchmark, I used the LLVM/Clang/Clangd code base which I 
> queried for all references of "std" (the namespace) which is around 46K 
> references in the index.
> 
> With end-loc in index: 3.45s on average (20 samples)
>  With end-loc computed on the fly: 11.33s on average (20 samples)
>  I also tried with Xcode but without too much success: it took about 30 secs 
> to reach 45K results and then carried on for a long time and hung (although I 
> didn't try to leave it for hours to see if it finished).



>> I think it's useful to highlight something even when the name is not there. 
>> For example in "MyClass o1, o2;" it feels natural that o1 and o2 would get 
>> highlighted.
> 
> To clarify, even with implicit references the start loc points to something. 
> In this case the implicit references can have start locs for the o1 and o2 
> identifiers and the end result for the UI will be the same (o1 and o2 get 
> highlighted) even without having end-locs for all references.

It's the same but slower. IMO, the trade off is not great. It's entirely 
subjective but I think 4-10 times slower in order to save an integer per 
occurrence is not worth it from my point of view.

> Even without end-loc, the start loc could point to MyStringRef and you could 
> highlight it.

(Same here, it's doable but faster if already in the index.)

>> It does? I can only seem to do a textual search.
> 
> The example I tried is the following. If you could file a bug report for the 
> test case that did not work as you expected it would be much appreciated!

Sure, I'll give that a try and isolate it as much as I can. BTW, does it work 
for you on the LLVM code base?


https://reviews.llvm.org/D39050



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

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



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

sammccall wrote:
> ioeric wrote:
> > s/this is symbol/this symbol/?
> Sorry, I hadn't seen this patch until now.
> When it was part of the `workspace/symbol` patch, I was the other person 
> concerned this was too coupled to existing use cases and preferred something 
> more descriptive.
> 
> I dug up an analysis @hokein and I worked on. One concluseion we came to was 
> that we thought results needed by `completion` were a subset of what 
> `workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc and 
> fragile though.
> 
> The cases we looked at were:
> 
> ||private|member|local|primary template|template specialization|nested in 
> template|
> | code complete |N|N|N|Y|N|N|
> | workspace/symbol |Y|Y|N|Y|Y|Y|
> | go to defn |Y|Y|?|?|?|?|
> (Y = index should return it, N = index should not return it, ? = don't care)
> 
> So the most relevant information seems to be:
>  - is this private (private member, internal linkage, no decl outside cpp 
> file, etc)
>  - is this nested in a class type (or template)
>  - is this a template specialization
> 
> I could live with bundling these into a single property (though they seem 
> like good ranking signals, and we'd lose them for that purpose), it will 
> certainly make a posting-list based index more efficient.
> 
> In that case I think we should have canonical documentation *somewhere* about 
> exactly what this subset is, and this field should reference that.
> e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and 
> `IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too many 
> different meanings - here we mean "index-based").
OK, I added documentation on the SymbolCollector (which was outdated) about 
what kinds of symbols are collected, with a reference to shouldFilterDecl. The 
subset is documented on isIndexedForCodeCompletion(), also referenced from the 
Symbol field. Was that more or less what you meant?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 149206.
malaperle added a comment.

Update with suggestions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+  return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
 
 namespace clang {
 namespace clangd {
@@ -132,9 +135,11 @@
 CollectorOpts, PragmaHandler.get());
 
 std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName,  TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11",   "-include",  TestHeaderName};
 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+
 tooling::ToolInvocation Invocation(
 Args,
 Factory->create(), Files.get(),
@@ -163,8 +168,20 @@
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   const std::string Header = R"(
 class Foo {
+  Foo() {}
+  Foo(int a) {}
   void f();
+  friend void f1();
+  friend class Friend;
+  Foo& operator=(const Foo&);
+  ~Foo();
+  class Nested {
+  void f();
+  };
+};
+class Friend {
 };
+
 void f1();
 inline void f2() {}
 static const int KInt = 2;
@@ -200,23 +217,78 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
   UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  {AllOf(QName("Foo"), ForCodeCompletion(true)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+   AllOf(QName("Friend"), ForCodeCompletion(true)),
+   AllOf(QName("f1"), ForCodeCompletion(true)),
+   AllOf(QName("f2"), ForCodeCompletion(true)),
+   AllOf(QName("KInt"), ForCodeCompletion(true)),
+   AllOf(QName("kStr"), ForCodeCompletion(true)),
+   AllOf(QName("foo"), ForCodeCompletion(true)),
+   AllOf(QName("foo::bar"), ForCodeCompletion(true)),
+   AllOf(QName("foo::int32"), ForCodeCompletion(true)),
+   AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
+   AllOf(QName("foo::v1"), ForCodeCompletion(true)),
+   AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),
+   AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T x = 0;};
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
 template <> struct Tmpl {};
 extern template struct Tmpl;
 template struct Tmpl;
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
-   QName("Tmpl"), DeclRange(Header.range()))}));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAreArray(
+  {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+   AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+  const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+  int foo;
+  ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategory)

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: unittests/clangd/CodeCompleteTests.cpp:741
 
+TEST(CompletionTest, Enums) {
+  EXPECT_THAT(completions(R"cpp(

ioeric wrote:
> malaperle wrote:
> > ioeric wrote:
> > > It's not clear to me what the following tests (`Enums`, 
> > > `AnonymousNamespace`, `InMainFile`) are testing. Do they test code 
> > > completion or  symbol collector? If these test symbol collection, they 
> > > should be moved int SymbolCollectorTest.cpp
> > They are testing that code completion works as intended regardless of how 
> > symbol collector is implemented. It's similar to our previous discussion in 
> > D44882 about "black box testing". I can remove them but it seems odd to me 
> > to not have code completion level tests for all cases because we assume 
> > that it will behave a certain way at the symbol collection and querying 
> > levels.
> FWIW, I am not against those tests at all; increasing coverage is always a 
> nice thing to do IMO. I just thought it would make more sense to add them in 
> a separate patch if they are not related to changes in this patch; I found 
> unrelated tests a bit confusing otherwise.
> I found unrelated tests a bit confusing otherwise.

Fair point!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 149532.
malaperle marked 4 inline comments as done.
malaperle added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+  return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
 
 namespace clang {
 namespace clangd {
@@ -132,9 +135,13 @@
 CollectorOpts, PragmaHandler.get());
 
 std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName,  TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11",   "-include",  TestHeaderName};
 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+// This allows to override the "-xc++" with something else, i.e.
+// -xobjective-c++.
+Args.push_back(TestFileName);
+
 tooling::ToolInvocation Invocation(
 Args,
 Factory->create(), Files.get(),
@@ -163,8 +170,20 @@
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   const std::string Header = R"(
 class Foo {
+  Foo() {}
+  Foo(int a) {}
   void f();
+  friend void f1();
+  friend class Friend;
+  Foo& operator=(const Foo&);
+  ~Foo();
+  class Nested {
+  void f();
+  };
+};
+class Friend {
 };
+
 void f1();
 inline void f2() {}
 static const int KInt = 2;
@@ -200,23 +219,78 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
   UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  {AllOf(QName("Foo"), ForCodeCompletion(true)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+   AllOf(QName("Friend"), ForCodeCompletion(true)),
+   AllOf(QName("f1"), ForCodeCompletion(true)),
+   AllOf(QName("f2"), ForCodeCompletion(true)),
+   AllOf(QName("KInt"), ForCodeCompletion(true)),
+   AllOf(QName("kStr"), ForCodeCompletion(true)),
+   AllOf(QName("foo"), ForCodeCompletion(true)),
+   AllOf(QName("foo::bar"), ForCodeCompletion(true)),
+   AllOf(QName("foo::int32"), ForCodeCompletion(true)),
+   AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
+   AllOf(QName("foo::v1"), ForCodeCompletion(true)),
+   AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),
+   AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T x = 0;};
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
 template <> struct Tmpl {};
 extern template struct Tmpl;
 template struct Tmpl;
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
-   QName("Tmpl"), DeclRange(Header.range()))}));
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAreArray(
+  {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+   AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+  const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)

[PATCH] D47643: Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D47643#1119187, @sammccall wrote:

> @malaperle: would you mind patching this in and checking whether attaching a 
> debugger still works on Mac (this was the reason for the EINTR loop, right?)
>
> I want to preserve this but now people other than me are complaining about 
> old clangds hanging around and eating all their CPU :-)


I tried the patch but as soon as I attach the debugger I get "IO error: 
Interrupted system call" from Clangd's stderr. I didn't get to look into this 
further.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47643



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

[PATCH] D47737: [clangd] Remove unused variables

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov, 
klimek.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47737

Files:
  clangd/ClangdServer.cpp


Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));


Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47643: Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D47643#1120913, @ilya-biryukov wrote:

> PS I've checked it on my Mac and lldb seems to attach just fine.


Working fine in my setup too!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47643



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

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE334017: [clangd] Add "member" symbols to the 
index (authored by malaperle, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D44954?vs=149532&id=149970#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44954

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  clangd/index/SymbolYAML.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -18,13 +18,18 @@
 namespace clang {
 namespace clangd {
 
-/// \brief Collect top-level symbols from an AST. These are symbols defined
-/// immediately inside a namespace or a translation unit scope. For example,
-/// symbols in classes or functions are not collected. Note that this only
-/// collects symbols that declared in at least one file that is not a main
-/// file (i.e. the source file corresponding to a TU). These are symbols that
-/// can be imported by other files by including the file where symbols are
-/// declared.
+/// \brief Collect declarations (symbols) from an AST.
+/// It collects most declarations except:
+/// - Implicit declarations
+/// - Anonymous declarations (anonymous enum/class/struct, etc)
+/// - Declarations in anonymous namespaces
+/// - Local declarations (in function bodies, blocks, etc)
+/// - Declarations in main files
+/// - Template specializations
+/// - Library-specific private declarations (e.g. private declaration generated
+/// by protobuf compiler)
+///
+/// See also shouldFilterDecl().
 ///
 /// Clients (e.g. clangd) can use SymbolCollector together with
 /// index::indexTopLevelDecls to retrieve all symbols when the source file is
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolCollector.h"
 #include "../AST.h"
+#include "../CodeComplete.h"
 #include "../CodeCompletionStrings.h"
 #include "../Logger.h"
 #include "../SourceCode.h"
@@ -149,21 +150,20 @@
   if (ND->isInAnonymousNamespace())
 return true;
 
-  // We only want:
-  //   * symbols in namespaces or translation unit scopes (e.g. no class
-  // members)
-  //   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
-  auto InTopLevelScope = hasDeclContext(
-  anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
-  // Don't index template specializations.
+  // We want most things but not "local" symbols such as symbols inside
+  // FunctionDecl, BlockDecl, ObjCMethodDecl and OMPDeclareReductionDecl.
+  // FIXME: Need a matcher for ExportDecl in order to include symbols declared
+  // within an export.
+  auto InNonLocalContext = hasDeclContext(anyOf(
+  translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+  enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+  objcCategoryImplDecl(), objcImplementationDecl()));
+  // Don't index template specializations and expansions in main files.
   auto IsSpecialization =
   anyOf(functionDecl(isExplicitTemplateSpecialization()),
 cxxRecordDecl(isExplicitTemplateSpecialization()),
 varDecl(isExplicitTemplateSpecialization()));
-  if (match(decl(allOf(unless(isExpansionInMainFile()),
-   anyOf(InTopLevelScope,
- hasDeclContext(enumDecl(InTopLevelScope,
- unless(isScoped(),
+  if (match(decl(allOf(unless(isExpansionInMainFile()), InNonLocalContext,
unless(IsSpecialization))),
 *ND, *ASTCtx)
   .empty())
@@ -377,6 +377,8 @@
   Symbol S;
   S.ID = std::move(ID);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+
+  S.IsIndexedForCodeCompletion = isIndexedForCodeCompletion(ND, Ctx);
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   if (auto DeclLoc =
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -149,9 +149,11 @@
   // The number of translation units that reference this symbol from their main
   // file. This number is only meaningful if aggregated in an index.
   unsigned References = 0;
-
+  /// Whether or not this symbol is meant to be used for the code completion.
+  /// See also isIndexedForCodeCompletion().
+  bool IsIndexedForCodeCompletion = false;
   /// A brief description of the symbol that can be displayed in the completion
- 

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

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL334017: [clangd] Add "member" symbols to the index 
(authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D44954

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.h
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/MemIndex.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.h
  clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -32,6 +32,7 @@
 using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
 
@@ -153,6 +154,7 @@
   Sym.CompletionSnippetInsertText = Sym.Name;
   Sym.CompletionLabel = Sym.Name;
   Sym.SymInfo.Kind = Kind;
+  Sym.IsIndexedForCodeCompletion = true;
   return Sym;
 }
 Symbol func(StringRef Name) { // Assumes the function has no args.
@@ -684,6 +686,20 @@
   Contains(AllOf(Named("baz"), Doc("Multi-line\nblock comment";
 }
 
+TEST(CompletionTest, GlobalCompletionFiltering) {
+
+  Symbol Class = cls("XYZ");
+  Class.IsIndexedForCodeCompletion = false;
+  Symbol Func = func("XYZ::f");
+  Func.IsIndexedForCodeCompletion = false;
+
+  auto Results = completions(R"(//  void f() {
+  XYZ::f^
+  })",
+ {Class, Func});
+  EXPECT_THAT(Results.items, IsEmpty());
+}
+
 TEST(CodeCompleteTest, DisableTypoCorrection) {
   auto Results = completions(R"cpp(
  namespace clang { int v; }
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
   Pos.end.character);
 }
 MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+  return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
 
 namespace clang {
 namespace clangd {
@@ -132,9 +135,13 @@
 CollectorOpts, PragmaHandler.get());
 
 std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName,  TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11",   "-include",  TestHeaderName};
 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+// This allows to override the "-xc++" with something else, i.e.
+// -xobjective-c++.
+Args.push_back(TestFileName);
+
 tooling::ToolInvocation Invocation(
 Args,
 Factory->create(), Files.get(),
@@ -163,8 +170,20 @@
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   const std::string Header = R"(
 class Foo {
+  Foo() {}
+  Foo(int a) {}
   void f();
+  friend void f1();
+  friend class Friend;
+  Foo& operator=(const Foo&);
+  ~Foo();
+  class Nested {
+  void f();
+  };
 };
+class Friend {
+};
+
 void f1();
 inline void f2() {}
 static const int KInt = 2;
@@ -200,23 +219,78 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
   UnorderedElementsAreArray(
-  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
-   QName("kStr"), QName("foo"), QName("foo::bar"),
-   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
-   QName("foo::bar::v2"), QName("foo::baz")}));
+  {AllOf(QName("Foo"), ForCodeCompletion(true)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+   AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+   AllOf(QName("Friend"), ForCodeCompletion(true)),
+   

[PATCH] D47737: [clangd] Remove unused variables

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE334018: [clangd] Remove unused variables (authored by 
malaperle, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47737?vs=149832&id=149976#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47737

Files:
  clangd/ClangdServer.cpp


Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));


Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47737: [clangd] Remove unused variables

[Marc-Andre Laperle via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334018: [clangd] Remove unused variables (authored by 
malaperle, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47737

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp


Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));


Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -298,9 +298,8 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, this](Callback> CB,
-llvm::Expected InpAST) {
+  auto Action = [Pos, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
@@ -391,10 +390,8 @@
 
 void ClangdServer::findDocumentHighlights(
 PathRef File, Position Pos, Callback> CB) {
-
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [FS, Pos](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
@@ -405,9 +402,8 @@
 
 void ClangdServer::findHover(PathRef File, Position Pos,
  Callback> CB) {
-  auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback> CB,
-  llvm::Expected InpAST) {
+  auto Action = [Pos](Callback> CB,
+  llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
 CB(clangd::getHover(InpAST->AST, Pos));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits