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 <[email protected]> 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 < > [email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
