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 McCall wrote:
>
>> I w
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 itself (not a symbol in it
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) u
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
> whether the namespac
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,
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 unit, yes. In multiple tr
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 too.
> But in that case
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 you give multiple C++ na
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.
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
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: at
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:
> There is a `SuppressUnwri
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
=
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
https://reviews.llvm.o
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 root
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
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 "a::b::
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 `SuppressUnwrittenScope` optio
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
unittests/clangd/SymbolCollectorT
19 matches
Mail list logo