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 <sammcc...@google.com> 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 <ibiryu...@google.com> > 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 <sammcc...@google.com> 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 <ibiryu...@google.com> >>> 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 <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 >>>> >>> >>> >> >> -- >> Regards, >> Ilya Biryukov >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits