Fixed prettyprinter in r324081 and USRs in r324093. On Fri, Feb 2, 2018 at 2:16 PM, Sam McCall <sammcc...@google.com> 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 <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