Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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 wrote: > I was misreading: we set isIgnored if we're trying to generate a USR for a > linkagespecdecl

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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)

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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 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

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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,

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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 wrote: > In a single translation

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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 wrote: > Right. And multiple TUs that *are* linked together would be fine

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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 wrote: > Yeah this is just a bug in clang's pprinter. I'll fix it. > > If

Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

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

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ioeric updated this revision to Diff 132549. ioeric marked 2 inline comments as done. ioeric edited the summary of this revision. ioeric added a comment. Addressed review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42796 Files: clangd/index/SymbolCollector.cpp

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ilya-biryukov added inline comments. Comment at: clangd/index/SymbolCollector.cpp:73 + Context = Context->getParent()) { +if (llvm::isa(Context) || +llvm::isa(Context)) ioeric wrote: > sammccall wrote: > > I'm not sure this is always correct:

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:69 +// qualifier. Inline namespaces and unscoped enums are skipped. +llvm::Expected getScope(const NamedDecl *ND) { + llvm::SmallVector Contexts; hokein wrote: >

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ioeric updated this revision to Diff 132552. ioeric added a comment. - clang-format Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42796 Files: clangd/index/SymbolCollector.cpp unittests/clangd/SymbolCollectorTests.cpp Index: unittests/clangd/SymbolCollectorTests.cpp

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

This revision was automatically updated to reflect the committed changes. Closed by commit rL324065: [clangd] Skip inline namespace when collecting scopes for index symbols. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ilya-biryukov added inline comments. Comment at: clangd/index/SymbolCollector.cpp:74 +if (llvm::isa(Context) || +llvm::isa(Context)) + break; I may not know enough about the AST, sorry if the question is obvious. `TranslationUnitDecl` is the

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

sammccall requested changes to this revision. sammccall added a comment. This revision now requires changes to proceed. Doh, nevermind - SuppressUnwrittenScopes is way simpler. Thanks @hokein for catching! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42796

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice catch, and nice fix! Might be worth adding a motivating example to the patch description. Comment at: clangd/index/SymbolCollector.cpp:68 +// For a symbol

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

hokein added inline comments. Comment at: clangd/index/SymbolCollector.cpp:69 +// qualifier. Inline namespaces and unscoped enums are skipped. +llvm::Expected getScope(const NamedDecl *ND) { + llvm::SmallVector Contexts; There is a

[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

ioeric created this revision. ioeric added reviewers: sammccall, hokein. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42796 Files: clangd/index/SymbolCollector.cpp