<div dir="ltr">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.</div><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Feb 2, 2018 at 1:45 PM Sam McCall <<a href="mailto:sammccall@google.com">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Right. And multiple TUs that *are* linked together would be fine too.<div>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.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov <span dir="ltr"><<a href="mailto:ibiryukov@google.com" target="_blank">ibiryukov@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>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).</div></div><div class="m_-7537597124780411743HOEnZb"><div class="m_-7537597124780411743h5"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Feb 2, 2018 at 1:04 PM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto">Yeah this is just a bug in clang's pprinter. I'll fix it.<div dir="auto"><br></div><div dir="auto">If you give multiple C++ names to the same linker symbol using extern C, I'm pretty sure you're in UB land.</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ilya-biryukov added inline comments.<br>
<br>
<br>
================<br>
Comment at: clangd/index/SymbolCollector.cpp:73<br>
+ Context = Context->getParent()) {<br>
+ if (llvm::isa<TranslationUnitDecl>(Context) ||<br>
+ llvm::isa<LinkageSpecDecl>(Context))<br>
----------------<br>
ioeric wrote:<br>
> sammccall wrote:<br>
> > I'm not sure this is always correct: at least clang accepts this code:<br>
> ><br>
> > namespace X { extern "C++" { int y; }}<br>
> ><br>
> > and you'll emit "y" instead of "X::y".<br>
> ><br>
> > I think the check you want is<br>
> ><br>
> > if (Context->isTransparentContext() || Context->isInlineNamespace())<br>
> > continue;<br>
> ><br>
> > isTransparentContext will handle the Namespace and Enum cases as you do below, including the enum/enum class distinction.<br>
> ><br>
> > (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)<br>
> 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`.<br>
><br>
> I also realized we've been dropping C symbols in `shouldFilterDecl` and fixed it in the same patch.<br>
I think we want `X::y`, not `y`.<br>
<br>
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:<br>
```<br>
namespace ns {<br>
extern "C" int foo();<br>
}<br>
<br>
void test() {<br>
ns::foo(); // ok<br>
foo(); // error<br>
::foo(); // error<br>
}<br>
```<br>
<br>
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.<br>
<br>
```<br>
namespace a {<br>
extern "C" int foo();<br>
}<br>
namespace b {<br>
extern "C" int foo(); // probably same USR, different qname. Also, possibly different types.<br>
}<br>
```<br>
<br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D42796" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D42796</a><br>
<br>
<br>
<br>
</blockquote></div>
</blockquote></div><br clear="all"><div><br></div></div></div><span class="m_-7537597124780411743HOEnZb"><font color="#888888">-- <br><div dir="ltr" class="m_-7537597124780411743m_-3296807356196679361gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</font></span></blockquote></div><br></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>