subject:"\[PATCH\] D42796\: \[clangd\] Skip inline namespace when collecting scopes for index symbols."

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

Fixed prettyprinter in r324081 and USRs in r324093.

On Fri, Feb 2, 2018 at 2:16 PM, Sam McCall  wrote:

> Talked to Ben, he thinks this is probably unintentional and that it's
> probably OK to change.
> I'll see if it breaks anything.
>
> On Fri, Feb 2, 2018 at 2:11 PM, Sam McCall  wrote:
>
>> I was misreading: we set isIgnored if we're trying to generate a USR for
>> a linkagespecdecl itself (not a symbol in it).
>> For other e.g. a var, we check if the DC is a NamedDecl and if so, visit
>> it before visiting the var.
>> Linkagespec isn't a nameddecl, so this is a no-op. Result: things
>> (directly) under extern {} blocks don't get any outer scope info in their
>> USR. But not sure if this is intended (it's certainly not what *we* want!)
>>
>> On Fri, Feb 2, 2018 at 2:05 PM, Ilya Biryukov 
>> wrote:
>>
>>> At least now we know they might cause problems. Thanks for digging into
>>> this.
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:53 PM Sam McCall  wrote:
>>>
 My intuition was that the USRs would be different, that linkage would
 either be included or not included from the USR, but it wouldn't affect
 whether the namespace is included. (Reasoning: USRs model language
 concepts, not linker ones)

 But we're both wrong. If I'm reading USRGeneration correctly, hitting a
 linkage spec while walking the scope tree sets the "ignore result" flag
 which signals the result is unusable (and short-circuits some paths that
 finish computing it). This seems like it may cause problems for us :-)
 I wonder why the tests didn't catch it, maybe I'm misreading.

 On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov 
 wrote:

> Exactly. We should make sure we *don't* treat them as the same symbol.
> But I would expect there USRs to be the same and that's what we use to
> deduplicate.
>
>
> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall 
> wrote:
>
>> Right. And multiple TUs that *are* linked together would be fine too.
>> But in that case I don't think we need to be clever about treating
>> these as the same symbol. Indexing them twice is fine.
>>
>> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
>> wrote:
>>
>>> In a single translation unit, yes. In multiple translation units
>>> that aren't linked together it's totally fine (may actually refer to
>>> different entities).
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall 
>>> wrote:
>>>
 Yeah this is just a bug in clang's pprinter. I'll fix it.

 If you give multiple C++ names to the same linker symbol using
 extern C, I'm pretty sure you're in UB land.

 On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
 revi...@reviews.llvm.org> wrote:

> ilya-biryukov added inline comments.
>
>
> 
> Comment at: clangd/index/SymbolCollector.cpp:73
> +   Context = Context->getParent()) {
> +if (llvm::isa(Context) ||
> +llvm::isa(Context))
> 
> ioeric wrote:
> > sammccall wrote:
> > > I'm not sure this is always correct: at least clang accepts
> this code:
> > >
> > >   namespace X { extern "C++" { int y; }}
> > >
> > > and you'll emit "y" instead of "X::y".
> > >
> > > I think the check you want is
> > >
> > >   if (Context->isTransparentContext() ||
> Context->isInlineNamespace())
> > > continue;
> > >
> > >  isTransparentContext will handle the Namespace and Enum cases
> as you do below, including the enum/enum class distinction.
> > >
> > > (The code you have below is otherwise correct, I think - but a
> reader needs to think about more separate cases in order to see that)
> > In `namespace X { extern "C++" { int y; }}`, we would still want
> `y` instead of `X::y` since C-style symbol doesn't have scope.
> `printQualifiedName` also does the same thing printing `y`; I've 
> added a
> test case for `extern C`.
> >
> > I also realized we've been dropping C symbols in
> `shouldFilterDecl` and fixed it in the same patch.
> I think we want `X::y`, not `y`.
>
> Lookup still finds it inside the namespace and does not find it in
> the global scope. So for our purposes they are actually inside the
> namespace and have the qualified name of this namespace. Here's an 
> example:
> ```
> namespace ns {
> extern "C" int foo();
> }
>
> void test() {
>   ns::foo(); // ok
>

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

Talked to Ben, he thinks this is probably unintentional and that it's
probably OK to change.
I'll see if it breaks anything.

On Fri, Feb 2, 2018 at 2:11 PM, Sam McCall  wrote:

> I was misreading: we set isIgnored if we're trying to generate a USR for a
> linkagespecdecl itself (not a symbol in it).
> For other e.g. a var, we check if the DC is a NamedDecl and if so, visit
> it before visiting the var.
> Linkagespec isn't a nameddecl, so this is a no-op. Result: things
> (directly) under extern {} blocks don't get any outer scope info in their
> USR. But not sure if this is intended (it's certainly not what *we* want!)
>
> On Fri, Feb 2, 2018 at 2:05 PM, Ilya Biryukov 
> wrote:
>
>> At least now we know they might cause problems. Thanks for digging into
>> this.
>>
>>
>> On Fri, Feb 2, 2018 at 1:53 PM Sam McCall  wrote:
>>
>>> My intuition was that the USRs would be different, that linkage would
>>> either be included or not included from the USR, but it wouldn't affect
>>> whether the namespace is included. (Reasoning: USRs model language
>>> concepts, not linker ones)
>>>
>>> But we're both wrong. If I'm reading USRGeneration correctly, hitting a
>>> linkage spec while walking the scope tree sets the "ignore result" flag
>>> which signals the result is unusable (and short-circuits some paths that
>>> finish computing it). This seems like it may cause problems for us :-)
>>> I wonder why the tests didn't catch it, maybe I'm misreading.
>>>
>>> On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov 
>>> wrote:
>>>
 Exactly. We should make sure we *don't* treat them as the same symbol.
 But I would expect there USRs to be the same and that's what we use to
 deduplicate.


 On Fri, Feb 2, 2018 at 1:45 PM Sam McCall  wrote:

> Right. And multiple TUs that *are* linked together would be fine too.
> But in that case I don't think we need to be clever about treating
> these as the same symbol. Indexing them twice is fine.
>
> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
> wrote:
>
>> In a single translation unit, yes. In multiple translation units that
>> aren't linked together it's totally fine (may actually refer to different
>> entities).
>>
>>
>> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall 
>> wrote:
>>
>>> Yeah this is just a bug in clang's pprinter. I'll fix it.
>>>
>>> If you give multiple C++ names to the same linker symbol using
>>> extern C, I'm pretty sure you're in UB land.
>>>
>>> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 ilya-biryukov added inline comments.


 
 Comment at: clangd/index/SymbolCollector.cpp:73
 +   Context = Context->getParent()) {
 +if (llvm::isa(Context) ||
 +llvm::isa(Context))
 
 ioeric wrote:
 > sammccall wrote:
 > > I'm not sure this is always correct: at least clang accepts
 this code:
 > >
 > >   namespace X { extern "C++" { int y; }}
 > >
 > > and you'll emit "y" instead of "X::y".
 > >
 > > I think the check you want is
 > >
 > >   if (Context->isTransparentContext() ||
 Context->isInlineNamespace())
 > > continue;
 > >
 > >  isTransparentContext will handle the Namespace and Enum cases
 as you do below, including the enum/enum class distinction.
 > >
 > > (The code you have below is otherwise correct, I think - but a
 reader needs to think about more separate cases in order to see that)
 > In `namespace X { extern "C++" { int y; }}`, we would still want
 `y` instead of `X::y` since C-style symbol doesn't have scope.
 `printQualifiedName` also does the same thing printing `y`; I've added 
 a
 test case for `extern C`.
 >
 > I also realized we've been dropping C symbols in
 `shouldFilterDecl` and fixed it in the same patch.
 I think we want `X::y`, not `y`.

 Lookup still finds it inside the namespace and does not find it in
 the global scope. So for our purposes they are actually inside the
 namespace and have the qualified name of this namespace. Here's an 
 example:
 ```
 namespace ns {
 extern "C" int foo();
 }

 void test() {
   ns::foo(); // ok
   foo(); // error
   ::foo(); // error
 }
 ```

 Note, however, that the tricky bit there is probably merging of the
 symbols, as it means symbols with the same USR (they are the same for 
 all

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

I was misreading: we set isIgnored if we're trying to generate a USR for a
linkagespecdecl itself (not a symbol in it).
For other e.g. a var, we check if the DC is a NamedDecl and if so, visit it
before visiting the var.
Linkagespec isn't a nameddecl, so this is a no-op. Result: things
(directly) under extern {} blocks don't get any outer scope info in their
USR. But not sure if this is intended (it's certainly not what *we* want!)

On Fri, Feb 2, 2018 at 2:05 PM, Ilya Biryukov  wrote:

> At least now we know they might cause problems. Thanks for digging into
> this.
>
>
> On Fri, Feb 2, 2018 at 1:53 PM Sam McCall  wrote:
>
>> My intuition was that the USRs would be different, that linkage would
>> either be included or not included from the USR, but it wouldn't affect
>> whether the namespace is included. (Reasoning: USRs model language
>> concepts, not linker ones)
>>
>> But we're both wrong. If I'm reading USRGeneration correctly, hitting a
>> linkage spec while walking the scope tree sets the "ignore result" flag
>> which signals the result is unusable (and short-circuits some paths that
>> finish computing it). This seems like it may cause problems for us :-)
>> I wonder why the tests didn't catch it, maybe I'm misreading.
>>
>> On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov 
>> wrote:
>>
>>> Exactly. We should make sure we *don't* treat them as the same symbol.
>>> But I would expect there USRs to be the same and that's what we use to
>>> deduplicate.
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall  wrote:
>>>
 Right. And multiple TUs that *are* linked together would be fine too.
 But in that case I don't think we need to be clever about treating
 these as the same symbol. Indexing them twice is fine.

 On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
 wrote:

> In a single translation unit, yes. In multiple translation units that
> aren't linked together it's totally fine (may actually refer to different
> entities).
>
>
> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall 
> wrote:
>
>> Yeah this is just a bug in clang's pprinter. I'll fix it.
>>
>> If you give multiple C++ names to the same linker symbol using extern
>> C, I'm pretty sure you're in UB land.
>>
>> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> ilya-biryukov added inline comments.
>>>
>>>
>>> 
>>> Comment at: clangd/index/SymbolCollector.cpp:73
>>> +   Context = Context->getParent()) {
>>> +if (llvm::isa(Context) ||
>>> +llvm::isa(Context))
>>> 
>>> ioeric wrote:
>>> > sammccall wrote:
>>> > > I'm not sure this is always correct: at least clang accepts this
>>> code:
>>> > >
>>> > >   namespace X { extern "C++" { int y; }}
>>> > >
>>> > > and you'll emit "y" instead of "X::y".
>>> > >
>>> > > I think the check you want is
>>> > >
>>> > >   if (Context->isTransparentContext() ||
>>> Context->isInlineNamespace())
>>> > > continue;
>>> > >
>>> > >  isTransparentContext will handle the Namespace and Enum cases
>>> as you do below, including the enum/enum class distinction.
>>> > >
>>> > > (The code you have below is otherwise correct, I think - but a
>>> reader needs to think about more separate cases in order to see that)
>>> > In `namespace X { extern "C++" { int y; }}`, we would still want
>>> `y` instead of `X::y` since C-style symbol doesn't have scope.
>>> `printQualifiedName` also does the same thing printing `y`; I've added a
>>> test case for `extern C`.
>>> >
>>> > I also realized we've been dropping C symbols in
>>> `shouldFilterDecl` and fixed it in the same patch.
>>> I think we want `X::y`, not `y`.
>>>
>>> Lookup still finds it inside the namespace and does not find it in
>>> the global scope. So for our purposes they are actually inside the
>>> namespace and have the qualified name of this namespace. Here's an 
>>> example:
>>> ```
>>> namespace ns {
>>> extern "C" int foo();
>>> }
>>>
>>> void test() {
>>>   ns::foo(); // ok
>>>   foo(); // error
>>>   ::foo(); // error
>>> }
>>> ```
>>>
>>> Note, however, that the tricky bit there is probably merging of the
>>> symbols, as it means symbols with the same USR (they are the same for 
>>> all
>>> `extern "c"` declarations with the same name, right?) can have different
>>> qualified names and we won't know which one to choose.
>>>
>>> ```
>>> namespace a {
>>>  extern "C" int foo();
>>> }
>>> namespace b {
>>>   extern "C" int foo(); // probably same USR, different qname. Also,
>>> possibly

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Ilya Biryukov via cfe-commits

At least now we know they might cause problems. Thanks for digging into
this.


On Fri, Feb 2, 2018 at 1:53 PM Sam McCall  wrote:

> My intuition was that the USRs would be different, that linkage would
> either be included or not included from the USR, but it wouldn't affect
> whether the namespace is included. (Reasoning: USRs model language
> concepts, not linker ones)
>
> But we're both wrong. If I'm reading USRGeneration correctly, hitting a
> linkage spec while walking the scope tree sets the "ignore result" flag
> which signals the result is unusable (and short-circuits some paths that
> finish computing it). This seems like it may cause problems for us :-)
> I wonder why the tests didn't catch it, maybe I'm misreading.
>
> On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov 
> wrote:
>
>> Exactly. We should make sure we *don't* treat them as the same symbol.
>> But I would expect there USRs to be the same and that's what we use to
>> deduplicate.
>>
>>
>> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall  wrote:
>>
>>> Right. And multiple TUs that *are* linked together would be fine too.
>>> But in that case I don't think we need to be clever about treating these
>>> as the same symbol. Indexing them twice is fine.
>>>
>>> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
>>> wrote:
>>>
 In a single translation unit, yes. In multiple translation units that
 aren't linked together it's totally fine (may actually refer to different
 entities).


 On Fri, Feb 2, 2018 at 1:04 PM Sam McCall  wrote:

> Yeah this is just a bug in clang's pprinter. I'll fix it.
>
> If you give multiple C++ names to the same linker symbol using extern
> C, I'm pretty sure you're in UB land.
>
> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> ilya-biryukov added inline comments.
>>
>>
>> 
>> Comment at: clangd/index/SymbolCollector.cpp:73
>> +   Context = Context->getParent()) {
>> +if (llvm::isa(Context) ||
>> +llvm::isa(Context))
>> 
>> ioeric wrote:
>> > sammccall wrote:
>> > > I'm not sure this is always correct: at least clang accepts this
>> code:
>> > >
>> > >   namespace X { extern "C++" { int y; }}
>> > >
>> > > and you'll emit "y" instead of "X::y".
>> > >
>> > > I think the check you want is
>> > >
>> > >   if (Context->isTransparentContext() ||
>> Context->isInlineNamespace())
>> > > continue;
>> > >
>> > >  isTransparentContext will handle the Namespace and Enum cases as
>> you do below, including the enum/enum class distinction.
>> > >
>> > > (The code you have below is otherwise correct, I think - but a
>> reader needs to think about more separate cases in order to see that)
>> > In `namespace X { extern "C++" { int y; }}`, we would still want
>> `y` instead of `X::y` since C-style symbol doesn't have scope.
>> `printQualifiedName` also does the same thing printing `y`; I've added a
>> test case for `extern C`.
>> >
>> > I also realized we've been dropping C symbols in `shouldFilterDecl`
>> and fixed it in the same patch.
>> I think we want `X::y`, not `y`.
>>
>> Lookup still finds it inside the namespace and does not find it in
>> the global scope. So for our purposes they are actually inside the
>> namespace and have the qualified name of this namespace. Here's an 
>> example:
>> ```
>> namespace ns {
>> extern "C" int foo();
>> }
>>
>> void test() {
>>   ns::foo(); // ok
>>   foo(); // error
>>   ::foo(); // error
>> }
>> ```
>>
>> Note, however, that the tricky bit there is probably merging of the
>> symbols, as it means symbols with the same USR (they are the same for all
>> `extern "c"` declarations with the same name, right?) can have different
>> qualified names and we won't know which one to choose.
>>
>> ```
>> namespace a {
>>  extern "C" int foo();
>> }
>> namespace b {
>>   extern "C" int foo(); // probably same USR, different qname. Also,
>> possibly different types.
>> }
>> ```
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D42796
>>
>>
>>
>>

 --
 Regards,
 Ilya Biryukov

>>>
>>>
>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>
>

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

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

My intuition was that the USRs would be different, that linkage would
either be included or not included from the USR, but it wouldn't affect
whether the namespace is included. (Reasoning: USRs model language
concepts, not linker ones)

But we're both wrong. If I'm reading USRGeneration correctly, hitting a
linkage spec while walking the scope tree sets the "ignore result" flag
which signals the result is unusable (and short-circuits some paths that
finish computing it). This seems like it may cause problems for us :-)
I wonder why the tests didn't catch it, maybe I'm misreading.

On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov  wrote:

> Exactly. We should make sure we *don't* treat them as the same symbol. But
> I would expect there USRs to be the same and that's what we use to
> deduplicate.
>
>
> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall  wrote:
>
>> Right. And multiple TUs that *are* linked together would be fine too.
>> But in that case I don't think we need to be clever about treating these
>> as the same symbol. Indexing them twice is fine.
>>
>> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
>> wrote:
>>
>>> In a single translation unit, yes. In multiple translation units that
>>> aren't linked together it's totally fine (may actually refer to different
>>> entities).
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall  wrote:
>>>
 Yeah this is just a bug in clang's pprinter. I'll fix it.

 If you give multiple C++ names to the same linker symbol using extern
 C, I'm pretty sure you're in UB land.

 On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
 revi...@reviews.llvm.org> wrote:

> ilya-biryukov added inline comments.
>
>
> 
> Comment at: clangd/index/SymbolCollector.cpp:73
> +   Context = Context->getParent()) {
> +if (llvm::isa(Context) ||
> +llvm::isa(Context))
> 
> ioeric wrote:
> > sammccall wrote:
> > > I'm not sure this is always correct: at least clang accepts this
> code:
> > >
> > >   namespace X { extern "C++" { int y; }}
> > >
> > > and you'll emit "y" instead of "X::y".
> > >
> > > I think the check you want is
> > >
> > >   if (Context->isTransparentContext() ||
> Context->isInlineNamespace())
> > > continue;
> > >
> > >  isTransparentContext will handle the Namespace and Enum cases as
> you do below, including the enum/enum class distinction.
> > >
> > > (The code you have below is otherwise correct, I think - but a
> reader needs to think about more separate cases in order to see that)
> > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
> instead of `X::y` since C-style symbol doesn't have scope.
> `printQualifiedName` also does the same thing printing `y`; I've added a
> test case for `extern C`.
> >
> > I also realized we've been dropping C symbols in `shouldFilterDecl`
> and fixed it in the same patch.
> I think we want `X::y`, not `y`.
>
> Lookup still finds it inside the namespace and does not find it in the
> global scope. So for our purposes they are actually inside the namespace
> and have the qualified name of this namespace. Here's an example:
> ```
> namespace ns {
> extern "C" int foo();
> }
>
> void test() {
>   ns::foo(); // ok
>   foo(); // error
>   ::foo(); // error
> }
> ```
>
> Note, however, that the tricky bit there is probably merging of the
> symbols, as it means symbols with the same USR (they are the same for all
> `extern "c"` declarations with the same name, right?) can have different
> qualified names and we won't know which one to choose.
>
> ```
> namespace a {
>  extern "C" int foo();
> }
> namespace b {
>   extern "C" int foo(); // probably same USR, different qname. Also,
> possibly different types.
> }
> ```
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D42796
>
>
>
>
>>>
>>> --
>>> Regards,
>>> Ilya Biryukov
>>>
>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

Right. And multiple TUs that *are* linked together would be fine too.
But in that case I don't think we need to be clever about treating these as
the same symbol. Indexing them twice is fine.

On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov  wrote:

> In a single translation unit, yes. In multiple translation units that
> aren't linked together it's totally fine (may actually refer to different
> entities).
>
>
> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall  wrote:
>
>> Yeah this is just a bug in clang's pprinter. I'll fix it.
>>
>> If you give multiple C++ names to the same linker symbol using extern C,
>> I'm pretty sure you're in UB land.
>>
>> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> ilya-biryukov added inline comments.
>>>
>>>
>>> 
>>> Comment at: clangd/index/SymbolCollector.cpp:73
>>> +   Context = Context->getParent()) {
>>> +if (llvm::isa(Context) ||
>>> +llvm::isa(Context))
>>> 
>>> ioeric wrote:
>>> > sammccall wrote:
>>> > > I'm not sure this is always correct: at least clang accepts this
>>> code:
>>> > >
>>> > >   namespace X { extern "C++" { int y; }}
>>> > >
>>> > > and you'll emit "y" instead of "X::y".
>>> > >
>>> > > I think the check you want is
>>> > >
>>> > >   if (Context->isTransparentContext() ||
>>> Context->isInlineNamespace())
>>> > > continue;
>>> > >
>>> > >  isTransparentContext will handle the Namespace and Enum cases as
>>> you do below, including the enum/enum class distinction.
>>> > >
>>> > > (The code you have below is otherwise correct, I think - but a
>>> reader needs to think about more separate cases in order to see that)
>>> > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
>>> instead of `X::y` since C-style symbol doesn't have scope.
>>> `printQualifiedName` also does the same thing printing `y`; I've added a
>>> test case for `extern C`.
>>> >
>>> > I also realized we've been dropping C symbols in `shouldFilterDecl`
>>> and fixed it in the same patch.
>>> I think we want `X::y`, not `y`.
>>>
>>> Lookup still finds it inside the namespace and does not find it in the
>>> global scope. So for our purposes they are actually inside the namespace
>>> and have the qualified name of this namespace. Here's an example:
>>> ```
>>> namespace ns {
>>> extern "C" int foo();
>>> }
>>>
>>> void test() {
>>>   ns::foo(); // ok
>>>   foo(); // error
>>>   ::foo(); // error
>>> }
>>> ```
>>>
>>> Note, however, that the tricky bit there is probably merging of the
>>> symbols, as it means symbols with the same USR (they are the same for all
>>> `extern "c"` declarations with the same name, right?) can have different
>>> qualified names and we won't know which one to choose.
>>>
>>> ```
>>> namespace a {
>>>  extern "C" int foo();
>>> }
>>> namespace b {
>>>   extern "C" int foo(); // probably same USR, different qname. Also,
>>> possibly different types.
>>> }
>>> ```
>>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D42796
>>>
>>>
>>>
>>>
>
> --
> Regards,
> Ilya Biryukov
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Ilya Biryukov via cfe-commits

Exactly. We should make sure we *don't* treat them as the same symbol. But
I would expect there USRs to be the same and that's what we use to
deduplicate.


On Fri, Feb 2, 2018 at 1:45 PM Sam McCall  wrote:

> Right. And multiple TUs that *are* linked together would be fine too.
> But in that case I don't think we need to be clever about treating these
> as the same symbol. Indexing them twice is fine.
>
> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov 
> wrote:
>
>> In a single translation unit, yes. In multiple translation units that
>> aren't linked together it's totally fine (may actually refer to different
>> entities).
>>
>>
>> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall  wrote:
>>
>>> Yeah this is just a bug in clang's pprinter. I'll fix it.
>>>
>>> If you give multiple C++ names to the same linker symbol using extern C,
>>> I'm pretty sure you're in UB land.
>>>
>>> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 ilya-biryukov added inline comments.


 
 Comment at: clangd/index/SymbolCollector.cpp:73
 +   Context = Context->getParent()) {
 +if (llvm::isa(Context) ||
 +llvm::isa(Context))
 
 ioeric wrote:
 > sammccall wrote:
 > > I'm not sure this is always correct: at least clang accepts this
 code:
 > >
 > >   namespace X { extern "C++" { int y; }}
 > >
 > > and you'll emit "y" instead of "X::y".
 > >
 > > I think the check you want is
 > >
 > >   if (Context->isTransparentContext() ||
 Context->isInlineNamespace())
 > > continue;
 > >
 > >  isTransparentContext will handle the Namespace and Enum cases as
 you do below, including the enum/enum class distinction.
 > >
 > > (The code you have below is otherwise correct, I think - but a
 reader needs to think about more separate cases in order to see that)
 > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
 instead of `X::y` since C-style symbol doesn't have scope.
 `printQualifiedName` also does the same thing printing `y`; I've added a
 test case for `extern C`.
 >
 > I also realized we've been dropping C symbols in `shouldFilterDecl`
 and fixed it in the same patch.
 I think we want `X::y`, not `y`.

 Lookup still finds it inside the namespace and does not find it in the
 global scope. So for our purposes they are actually inside the namespace
 and have the qualified name of this namespace. Here's an example:
 ```
 namespace ns {
 extern "C" int foo();
 }

 void test() {
   ns::foo(); // ok
   foo(); // error
   ::foo(); // error
 }
 ```

 Note, however, that the tricky bit there is probably merging of the
 symbols, as it means symbols with the same USR (they are the same for all
 `extern "c"` declarations with the same name, right?) can have different
 qualified names and we won't know which one to choose.

 ```
 namespace a {
  extern "C" int foo();
 }
 namespace b {
   extern "C" int foo(); // probably same USR, different qname. Also,
 possibly different types.
 }
 ```


 Repository:
   rL LLVM

 https://reviews.llvm.org/D42796




>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>
>

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

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Ilya Biryukov via cfe-commits

In a single translation unit, yes. In multiple translation units that
aren't linked together it's totally fine (may actually refer to different
entities).


On Fri, Feb 2, 2018 at 1:04 PM Sam McCall  wrote:

> Yeah this is just a bug in clang's pprinter. I'll fix it.
>
> If you give multiple C++ names to the same linker symbol using extern C,
> I'm pretty sure you're in UB land.
>
> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> ilya-biryukov added inline comments.
>>
>>
>> 
>> Comment at: clangd/index/SymbolCollector.cpp:73
>> +   Context = Context->getParent()) {
>> +if (llvm::isa(Context) ||
>> +llvm::isa(Context))
>> 
>> ioeric wrote:
>> > sammccall wrote:
>> > > I'm not sure this is always correct: at least clang accepts this code:
>> > >
>> > >   namespace X { extern "C++" { int y; }}
>> > >
>> > > and you'll emit "y" instead of "X::y".
>> > >
>> > > I think the check you want is
>> > >
>> > >   if (Context->isTransparentContext() || Context->isInlineNamespace())
>> > > continue;
>> > >
>> > >  isTransparentContext will handle the Namespace and Enum cases as you
>> do below, including the enum/enum class distinction.
>> > >
>> > > (The code you have below is otherwise correct, I think - but a reader
>> needs to think about more separate cases in order to see that)
>> > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
>> instead of `X::y` since C-style symbol doesn't have scope.
>> `printQualifiedName` also does the same thing printing `y`; I've added a
>> test case for `extern C`.
>> >
>> > I also realized we've been dropping C symbols in `shouldFilterDecl` and
>> fixed it in the same patch.
>> I think we want `X::y`, not `y`.
>>
>> Lookup still finds it inside the namespace and does not find it in the
>> global scope. So for our purposes they are actually inside the namespace
>> and have the qualified name of this namespace. Here's an example:
>> ```
>> namespace ns {
>> extern "C" int foo();
>> }
>>
>> void test() {
>>   ns::foo(); // ok
>>   foo(); // error
>>   ::foo(); // error
>> }
>> ```
>>
>> Note, however, that the tricky bit there is probably merging of the
>> symbols, as it means symbols with the same USR (they are the same for all
>> `extern "c"` declarations with the same name, right?) can have different
>> qualified names and we won't know which one to choose.
>>
>> ```
>> namespace a {
>>  extern "C" int foo();
>> }
>> namespace b {
>>   extern "C" int foo(); // probably same USR, different qname. Also,
>> possibly different types.
>> }
>> ```
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D42796
>>
>>
>>
>>

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

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Sam McCall via cfe-commits

Yeah this is just a bug in clang's pprinter. I'll fix it.

If you give multiple C++ names to the same linker symbol using extern C,
I'm pretty sure you're in UB land.

On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
revi...@reviews.llvm.org> wrote:

> ilya-biryukov added inline comments.
>
>
> 
> Comment at: clangd/index/SymbolCollector.cpp:73
> +   Context = Context->getParent()) {
> +if (llvm::isa(Context) ||
> +llvm::isa(Context))
> 
> ioeric wrote:
> > sammccall wrote:
> > > I'm not sure this is always correct: at least clang accepts this code:
> > >
> > >   namespace X { extern "C++" { int y; }}
> > >
> > > and you'll emit "y" instead of "X::y".
> > >
> > > I think the check you want is
> > >
> > >   if (Context->isTransparentContext() || Context->isInlineNamespace())
> > > continue;
> > >
> > >  isTransparentContext will handle the Namespace and Enum cases as you
> do below, including the enum/enum class distinction.
> > >
> > > (The code you have below is otherwise correct, I think - but a reader
> needs to think about more separate cases in order to see that)
> > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
> instead of `X::y` since C-style symbol doesn't have scope.
> `printQualifiedName` also does the same thing printing `y`; I've added a
> test case for `extern C`.
> >
> > I also realized we've been dropping C symbols in `shouldFilterDecl` and
> fixed it in the same patch.
> I think we want `X::y`, not `y`.
>
> Lookup still finds it inside the namespace and does not find it in the
> global scope. So for our purposes they are actually inside the namespace
> and have the qualified name of this namespace. Here's an example:
> ```
> namespace ns {
> extern "C" int foo();
> }
>
> void test() {
>   ns::foo(); // ok
>   foo(); // error
>   ::foo(); // error
> }
> ```
>
> Note, however, that the tricky bit there is probably merging of the
> symbols, as it means symbols with the same USR (they are the same for all
> `extern "c"` declarations with the same name, right?) can have different
> qualified names and we won't know which one to choose.
>
> ```
> namespace a {
>  extern "C" int foo();
> }
> namespace b {
>   extern "C" int foo(); // probably same USR, different qname. Also,
> possibly different types.
> }
> ```
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D42796
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Eric Liu via Phabricator via cfe-commits

ioeric updated this revision to Diff 132549.
ioeric marked 2 inline comments as done.
ioeric edited the summary of this revision.
ioeric added a comment.

Addressed review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796

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

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -201,8 +201,7 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
 QName("Green"), QName("Color2"),
-QName("ns"),
-QName("ns::Black")));
+QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
@@ -324,6 +323,53 @@
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(
+namespace na {
+class Foo {};
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::nb::Bar")));
+}
+
+TEST_F(SymbolCollectorTest, ExternC) {
+  const std::string Header = R"(
+extern "C" { class Foo {}; }
+namespace na {
+extern "C" { class Bar {}; }
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("na"), QName("Foo"),
+QName("Bar")));
+}
+
+TEST_F(SymbolCollectorTest, SkipInlineNamespace) {
+  const std::string Header = R"(
+namespace na {
+inline namespace nb {
+class Foo {};
+}
+}
+namespace na {
+// This is still inlined.
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::Bar")));
+}
+
 TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
   const std::string Header = R"(
 namespace nx {
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
+#include "Logger.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
@@ -97,8 +98,8 @@
   //   * 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()));
+  auto InTopLevelScope = hasDeclContext(
+  anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
   if (match(decl(allOf(Opts.IndexMainFiles
? decl()
: decl(unless(isExpansionInMainFile())),
@@ -180,7 +181,17 @@
   return true;
 
 auto  = ND->getASTContext().getSourceManager();
-std::string QName = ND->getQualifiedNameAsString();
+
+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();
 
 Symbol S;
 S.ID = std::move(ID);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:73
+   Context = Context->getParent()) {
+if (llvm::isa(Context) ||
+llvm::isa(Context))

ioeric wrote:
> sammccall wrote:
> > I'm not sure this is always correct: at least clang accepts this code:
> > 
> >   namespace X { extern "C++" { int y; }}
> > 
> > and you'll emit "y" instead of "X::y".
> > 
> > I think the check you want is
> > 
> >   if (Context->isTransparentContext() || Context->isInlineNamespace())
> > continue;
> > 
> >  isTransparentContext will handle the Namespace and Enum cases as you do 
> > below, including the enum/enum class distinction.
> > 
> > (The code you have below is otherwise correct, I think - but a reader needs 
> > to think about more separate cases in order to see that)
> In `namespace X { extern "C++" { int y; }}`, we would still want `y` instead 
> of `X::y` since C-style symbol doesn't have scope. `printQualifiedName` also 
> does the same thing printing `y`; I've added a test case for `extern C`.
> 
> I also realized we've been dropping C symbols in `shouldFilterDecl` and fixed 
> it in the same patch.
I think we want `X::y`, not `y`.

Lookup still finds it inside the namespace and does not find it in the global 
scope. So for our purposes they are actually inside the namespace and have the 
qualified name of this namespace. Here's an example:
```
namespace ns {
extern "C" int foo();
}

void test() {
  ns::foo(); // ok
  foo(); // error
  ::foo(); // error
}
```

Note, however, that the tricky bit there is probably merging of the symbols, as 
it means symbols with the same USR (they are the same for all `extern "c"` 
declarations with the same name, right?) can have different qualified names and 
we won't know which one to choose.

```
namespace a {
 extern "C" int foo();
}
namespace b {
  extern "C" int foo(); // probably same USR, different qname. Also, possibly 
different types.
}
```


Repository:
  rL LLVM

https://reviews.llvm.org/D42796



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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Eric Liu via Phabricator via cfe-commits

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:69
+// qualifier. Inline namespaces and unscoped enums are skipped.
+llvm::Expected getScope(const NamedDecl *ND) {
+  llvm::SmallVector Contexts;

hokein wrote:
> There is a `SuppressUnwrittenScope` option in `PrintingPolicy`,  I think we 
> can probably use `printQualifiedName` with our customized policy (setting 
> `SuppressUnwrittenScope` to true) here.
This is perfect! Thank you!



Comment at: clangd/index/SymbolCollector.cpp:73
+   Context = Context->getParent()) {
+if (llvm::isa(Context) ||
+llvm::isa(Context))

sammccall wrote:
> I'm not sure this is always correct: at least clang accepts this code:
> 
>   namespace X { extern "C++" { int y; }}
> 
> and you'll emit "y" instead of "X::y".
> 
> I think the check you want is
> 
>   if (Context->isTransparentContext() || Context->isInlineNamespace())
> continue;
> 
>  isTransparentContext will handle the Namespace and Enum cases as you do 
> below, including the enum/enum class distinction.
> 
> (The code you have below is otherwise correct, I think - but a reader needs 
> to think about more separate cases in order to see that)
In `namespace X { extern "C++" { int y; }}`, we would still want `y` instead of 
`X::y` since C-style symbol doesn't have scope. `printQualifiedName` also does 
the same thing printing `y`; I've added a test case for `extern C`.

I also realized we've been dropping C symbols in `shouldFilterDecl` and fixed 
it in the same patch.



Comment at: clangd/index/SymbolCollector.cpp:74
+if (llvm::isa(Context) ||
+llvm::isa(Context))
+  break;

ilya-biryukov wrote:
> I may not know enough about the AST, sorry if the question is obvious.
> `TranslationUnitDecl` is the root of the tree, but why should we stop at 
> `LinkageSpecDecl`?
> 
> This code is probably going away per @hokein's comments.
Symbols in `LinkageSpecDecl` (i.e. `extern "C"`) are C style symbols and do not 
have scopes. Also see my reply to Sam's related comment.



Comment at: clangd/index/SymbolCollector.cpp:195
 llvm::SmallString<128> USR;
+if (ND->getIdentifier() == nullptr)
+  return true;

ilya-biryukov wrote:
> sammccall wrote:
> > hokein wrote:
> > > Consider moving to `shouldFilterDecl`? We also have a check `if 
> > > (ND->getDeclName().isEmpty())` there, which I assume does similar thing. 
> > hmm, what case is this handling? should `shouldFilterDecl` catch it?
> Why do we skip names without identifiers? AFAIK, they are perfectly 
> reasonable C++ entities: overloaded operators, constructors, etc. 
`getName` crashes for `NamedDecl` without identifier. I thought symbols without 
identifier are not interesting for global code completion, so I added this 
filter to avoid crash. But on a second thought, these symbols would still be 
useful for go-to-definition, for example. 

This is no longer needed with `printQualifiedName` though.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796



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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Eric Liu via Phabricator via cfe-commits

ioeric updated this revision to Diff 132552.
ioeric added a comment.

- clang-format


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796

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

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -201,8 +201,7 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
 QName("Green"), QName("Color2"),
-QName("ns"),
-QName("ns::Black")));
+QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
@@ -324,6 +323,53 @@
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(
+namespace na {
+class Foo {};
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::nb::Bar")));
+}
+
+TEST_F(SymbolCollectorTest, ExternC) {
+  const std::string Header = R"(
+extern "C" { class Foo {}; }
+namespace na {
+extern "C" { class Bar {}; }
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("Foo"), QName("Bar")));
+}
+
+TEST_F(SymbolCollectorTest, SkipInlineNamespace) {
+  const std::string Header = R"(
+namespace na {
+inline namespace nb {
+class Foo {};
+}
+}
+namespace na {
+// This is still inlined.
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::Bar")));
+}
+
 TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
   const std::string Header = R"(
 namespace nx {
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
+#include "Logger.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
@@ -97,8 +98,8 @@
   //   * 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()));
+  auto InTopLevelScope = hasDeclContext(
+  anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
   if (match(decl(allOf(Opts.IndexMainFiles
? decl()
: decl(unless(isExpansionInMainFile())),
@@ -180,7 +181,17 @@
   return true;
 
 auto  = ND->getASTContext().getSourceManager();
-std::string QName = ND->getQualifiedNameAsString();
+
+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();
 
 Symbol S;
 S.ID = std::move(ID);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Eric Liu via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324065: [clangd] Skip inline namespace when collecting 
scopes for index symbols. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D42796

Files:
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

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
@@ -198,8 +198,7 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
 QName("Green"), QName("Color2"),
-QName("ns"),
-QName("ns::Black")));
+QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
@@ -321,6 +320,53 @@
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(
+namespace na {
+class Foo {};
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::nb::Bar")));
+}
+
+TEST_F(SymbolCollectorTest, ExternC) {
+  const std::string Header = R"(
+extern "C" { class Foo {}; }
+namespace na {
+extern "C" { class Bar {}; }
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("Foo"), QName("Bar")));
+}
+
+TEST_F(SymbolCollectorTest, SkipInlineNamespace) {
+  const std::string Header = R"(
+namespace na {
+inline namespace nb {
+class Foo {};
+}
+}
+namespace na {
+// This is still inlined.
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::Bar")));
+}
+
 TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
   const std::string Header = R"(
 namespace nx {
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
+#include "Logger.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
@@ -97,8 +98,8 @@
   //   * 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()));
+  auto InTopLevelScope = hasDeclContext(
+  anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
   if (match(decl(allOf(Opts.IndexMainFiles
? decl()
: decl(unless(isExpansionInMainFile())),
@@ -180,7 +181,17 @@
   return true;
 
 auto  = ND->getASTContext().getSourceManager();
-std::string QName = ND->getQualifiedNameAsString();
+
+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();
 
 Symbol S;
 S.ID = std::move(ID);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-02 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:74
+if (llvm::isa(Context) ||
+llvm::isa(Context))
+  break;

I may not know enough about the AST, sorry if the question is obvious.
`TranslationUnitDecl` is the root of the tree, but why should we stop at 
`LinkageSpecDecl`?

This code is probably going away per @hokein's comments.



Comment at: clangd/index/SymbolCollector.cpp:195
 llvm::SmallString<128> USR;
+if (ND->getIdentifier() == nullptr)
+  return true;

sammccall wrote:
> hokein wrote:
> > Consider moving to `shouldFilterDecl`? We also have a check `if 
> > (ND->getDeclName().isEmpty())` there, which I assume does similar thing. 
> hmm, what case is this handling? should `shouldFilterDecl` catch it?
Why do we skip names without identifiers? AFAIK, they are perfectly reasonable 
C++ entities: overloaded operators, constructors, etc. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796



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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-01 Thread Sam McCall via Phabricator via cfe-commits

sammccall requested changes to this revision.
sammccall added a comment.
This revision now requires changes to proceed.

Doh, nevermind - SuppressUnwrittenScopes is way simpler. Thanks @hokein for 
catching!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796



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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-01 Thread Sam McCall via Phabricator via cfe-commits

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

Nice catch, and nice fix! Might be worth adding a motivating example to the 
patch description.

Comment at: clangd/index/SymbolCollector.cpp:68
+// For a symbol "a::b::c", return "a::b::". Scope is empty if there's no
+// qualifier. Inline namespaces and unscoped enums are skipped.
+llvm::Expected getScope(const NamedDecl *ND) {

I'd expand on this for inline namespaces a little, because it's non-obvious. 
e.g.

  // 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.

Conversely, I don't think you need to explicitly mention unscoped enums 
anymore, because the behavior for transparent scopes is the obvious one, and we 
shouldn't need any special code handling enums (see below suggestions).

Comment at: clangd/index/SymbolCollector.cpp:71
+  llvm::SmallVector Contexts;
+  for (const auto *Context = ND->getDeclContext(); Context;
+   Context = Context->getParent()) {

if the condition is `!Context->isTranslationUnit()` you can skip the break 
inside, which I think reads more naturally. You'll never reach null - only TU 
can have a null parent I think.

Comment at: clangd/index/SymbolCollector.cpp:71
+  llvm::SmallVector Contexts;
+  for (const auto *Context = ND->getDeclContext(); Context;
+   Context = Context->getParent()) {

sammccall wrote:
> if the condition is `!Context->isTranslationUnit()` you can skip the break 
> inside, which I think reads more naturally. You'll never reach null - only TU 
> can have a null parent I think.
uber-nit: I think `DC` is pretty common in clang to refer to a DeclContext - 
once I got used to it, it seems less ambiguous than Context. Up to you though.

Comment at: clangd/index/SymbolCollector.cpp:73
+   Context = Context->getParent()) {
+if (llvm::isa(Context) ||
+llvm::isa(Context))

I'm not sure this is always correct: at least clang accepts this code:

  namespace X { extern "C++" { int y; }}

and you'll emit "y" instead of "X::y".

I think the check you want is

  if (Context->isTransparentContext() || Context->isInlineNamespace())
continue;

 isTransparentContext will handle the Namespace and Enum cases as you do below, 
including the enum/enum class distinction.

(The code you have below is otherwise correct, I think - but a reader needs to 
think about more separate cases in order to see that)

Comment at: clangd/index/SymbolCollector.cpp:76
+  break;
+
+if (const auto *NSD = dyn_cast(Context)) {

With the changes suggested above, I think we only get to this point in these 
cases:
 1. non-inline namespace
 2. decl is in some other named scope (class, scoped enum, ...)

Currently case 2 is symbols we're not indexing: shouldFilterDecl() should be 
false. So this is a programming error. So I think we just want

  Contexts.push_back(cast(Context)->getName());

which includes an assertion. Returning Expected seems weird here - we should 
never hit it unless a precondition is violated.

Comment at: clangd/index/SymbolCollector.cpp:90
+  }
+  std::string Scope = llvm::join(Contexts.rbegin(), Contexts.rend(), "::");
+  if (!Scope.empty())

(nit: might be slightly more obvious just to write the for loop and avoid the 
special case, up to you)

Comment at: clangd/index/SymbolCollector.cpp:113
   // violations.
   if (ND->isInAnonymousNamespace())
 return true;

Hmm, if this is ever hot-path, we may want to eventually combine "determine 
scope" and "shouldFilter" somewhat. shouldFilterDecl is doing much the same 
upwards-scope-traversal, and it seems pretty redundant.

Nothing to do for now though.

Comment at: clangd/index/SymbolCollector.cpp:195
 llvm::SmallString<128> USR;
+if (ND->getIdentifier() == nullptr)
+  return true;

hokein wrote:
> Consider moving to `shouldFilterDecl`? We also have a check `if 
> (ND->getDeclName().isEmpty())` there, which I assume does similar thing. 
hmm, what case is this handling? should `shouldFilterDecl` catch it?

Comment at: unittests/clangd/SymbolCollectorTests.cpp:326

+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(

you could consider modifying one of the testcases to have a weird 
linkage-spec-inside-namespace thing I mentioned :-)

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796

___
cfe-commits mailing list

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-01 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:69
+// qualifier. Inline namespaces and unscoped enums are skipped.
+llvm::Expected getScope(const NamedDecl *ND) {
+  llvm::SmallVector Contexts;

There is a `SuppressUnwrittenScope` option in `PrintingPolicy`,  I think we can 
probably use `printQualifiedName` with our customized policy (setting 
`SuppressUnwrittenScope` to true) here.



Comment at: clangd/index/SymbolCollector.cpp:195
 llvm::SmallString<128> USR;
+if (ND->getIdentifier() == nullptr)
+  return true;

Consider moving to `shouldFilterDecl`? We also have a check `if 
(ND->getDeclName().isEmpty())` there, which I assume does similar thing. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796



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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

2018-02-01 Thread Eric Liu via Phabricator via cfe-commits

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

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42796

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

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -201,8 +201,7 @@
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
 QName("Green"), QName("Color2"),
-QName("ns"),
-QName("ns::Black")));
+QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
@@ -324,6 +323,41 @@
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(
+namespace na {
+class Foo {};
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::nb::Bar")));
+}
+
+TEST_F(SymbolCollectorTest, SkipInlineNamespace) {
+  const std::string Header = R"(
+namespace na {
+inline namespace nb {
+class Foo {};
+}
+}
+namespace na {
+// This is still inlined.
+namespace nb {
+class Bar {};
+}
+}
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("na"), QName("na::nb"),
+   QName("na::Foo"), QName("na::Bar")));
+}
+
 TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
   const std::string Header = R"(
 namespace nx {
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
+#include "Logger.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
@@ -63,14 +64,33 @@
   return AbsolutePath.str();
 }
 
-// "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no qualifier.
-std::pair
-splitQualifiedName(llvm::StringRef QName) {
-  assert(!QName.startswith("::") && "Qualified names should not start with ::");
-  size_t Pos = QName.rfind("::");
-  if (Pos == llvm::StringRef::npos)
-return {StringRef(), QName};
-  return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
+// For a symbol "a::b::c", return "a::b::". Scope is empty if there's no
+// qualifier. Inline namespaces and unscoped enums are skipped.
+llvm::Expected getScope(const NamedDecl *ND) {
+  llvm::SmallVector Contexts;
+  for (const auto *Context = ND->getDeclContext(); Context;
+   Context = Context->getParent()) {
+if (llvm::isa(Context) ||
+llvm::isa(Context))
+  break;
+
+if (const auto *NSD = dyn_cast(Context)) {
+  if (!NSD->isInlineNamespace())
+Contexts.push_back(NSD->getName());
+} else if (const auto *ED = dyn_cast(Context)) {
+  if (ED->isScoped())
+Contexts.push_back(ED->getName());
+} else {
+  return llvm::make_error(
+  llvm::Twine("Unexpected context type ") + Context->getDeclKindName() +
+  " for symbol " + ND->getQualifiedNameAsString(),
+  llvm::inconvertibleErrorCode());
+}
+  }
+  std::string Scope = llvm::join(Contexts.rbegin(), Contexts.rend(), "::");
+  if (!Scope.empty())
+Scope.append("::");
+  return Scope;
 }
 
 bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
@@ -172,19 +192,27 @@
 if (shouldFilterDecl(ND, ASTCtx, Opts))
   return true;
 llvm::SmallString<128> USR;
+if (ND->getIdentifier() == nullptr)
+  return true;
 if (index::generateUSRForDecl(ND, USR))
   return true;
 
 auto ID = SymbolID(USR);
 if (Symbols.find(ID) != nullptr)
   return true;
 
 auto  = ND->getASTContext().getSourceManager();
-std::string QName = ND->getQualifiedNameAsString();
+auto Scope = getScope(ND);
+if (!Scope) {
+  log(llvm::toString(Scope.takeError()));
+  return true;
+}
+std::string Name = ND->getName();
 
 Symbol S;
 S.ID = std::move(ID);
-std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+S.Scope = *Scope;
+S.Name = Name;
 S.SymInfo = index::getSymbolInfo(D);
 std::string FilePath;
 S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir,

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

19 matches

Site Navigation

Mail list logo

Footer information