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 <sammcc...@google.com> 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 <ibiryu...@google.com> > 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 <sammcc...@google.com> 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<TranslationUnitDecl>(Context) || >>>> + llvm::isa<LinkageSpecDecl>(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