This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add a flag for spelled references in the Index
ClosedPublic

Authored by kbobyrev on Jan 14 2020, 6:25 PM.

Details

Summary

This patch allows the index does to provide a way to distinguish implicit references (e.g. coming from macro expansions) from the spelled ones. The corresponding flag was added to RefKind and symbols that are referenced without spelling their name explicitly are now marked implicit. This allows fixing incorrect behavior when renaming a symbol that was referenced in macro expansions would try to rename macro invocations.

Diff Detail

Event Timeline

kbobyrev created this revision.Jan 14 2020, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 6:25 PM

The patch could be shorter and slightly less confusing if I preserved 1:1 RefKind::Implicit <-> index::SymbolKind::Implicit relation, but I thought we might want to keep RefKind being 1 byte so that we don't waste unnecessary memory. Also, since we might want to start using findExplicitReferences (or something else we come up with) for indexing, it shouldn't be critical. Let me know if you think it's unnecessary.

+CC @hokein

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

kadircet added inline comments.Jan 15 2020, 3:31 AM
clang-tools-extra/clangd/index/Ref.h
33

instead of doing that, could we rather de-couple two enums completely and have a symbolRoleToRefKind(and vice-versa) method instead(we already have some customization in toRefKind now) ?

as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)

clang-tools-extra/clangd/index/SymbolCollector.cpp
297

this might be expensive to trigger while indexing for each reference.

could we rather do it as post-processing inside SymbolCollector::finish while making use of TokenBuffer::spelledTokens(you can create a tokenbuffer through TokenCollector with the PP inside SymbolCollector.)

300

It seems like we are only checking for syntactic occurence of a symbol. Can we make use of Spelled Rather than Implicit (which has some semantic meaning) to make that clear?

kadircet added inline comments.Jan 15 2020, 3:33 AM
clang-tools-extra/clangd/index/Ref.h
33

note that this would also require a bump to version of on-disk index in clangd/index/Serialization.cpp, as old information regarding RefKind won't be usable anymore.

The patch could be shorter and slightly less confusing if I preserved 1:1 RefKind::Implicit <-> index::SymbolKind::Implicit relation, but I thought we might want to keep RefKind being 1 byte so that we don't waste unnecessary memory.

I think it is time to create our own Declaration, Definition in our RefKind (rather than reusing the enums in libindex).

Also, since we might want to start using findExplicitReferences (or something else we come up with) for indexing, it shouldn't be critical. Let me know if you think it's unnecessary.

Yeah, we may have a plan in the future, but I don't think switching to findExplicitReferences is trivial, there are a few things we need to figure out carefully, e.g. how to support macros, implicit references.

clang-tools-extra/clangd/index/Ref.h
30–66

I'm skeptical about definition of index::SymbolRole::Implicit.

I think the implicit we want here is references that are spelled/written as the same as the named decls, so it is more likely to align with index::SymbolRole::NameReferences.

clang-tools-extra/clangd/index/SymbolCollector.cpp
300

+1, Implicit is ambiguous, maybe Named, Explicit (this reflects to the findExplicitReferences)?

kbobyrev updated this revision to Diff 238786.Jan 17 2020, 8:16 AM
kbobyrev marked 4 inline comments as done.

Address alsmost all comments.

clang-tools-extra/clangd/index/Ref.h
33

instead of doing that, could we rather de-couple two enums completely and have a symbolRoleToRefKind(and vice-versa) method instead(we already have some customization in toRefKind now) ?
as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd)

Makes sense, will do!

note that this would also require a bump to version of on-disk index in clangd/index/Serialization.cpp, as old information regarding RefKind won't be usable anymore.

I checked the Serialization code and the serialization code should be OK as long as RefKind stays one byte. Can you please elaborate on this?

Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)
Or do you mean there should be some other change in the code for me to do to land this patch?

kbobyrev planned changes to this revision.Jan 17 2020, 8:18 AM

Need to make use of the TokenBuffer, ran into some difficulties when lexing some files and triggering assertions in the process. Will figure it out and update the patch.

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

kbobyrev updated this revision to Diff 238902.Jan 17 2020, 3:19 PM

I started using TokenBuffer, but I ran into the following issue: when I'm
creating TokenBuffer and TokenCollector, they do not contain any tokens.
Preprocessor does not seem to have a non-null Lexer instance, TokenWatcher
(set in TokenCollector) is not triggered anywhere and neither is
Lexer::Lex. I don't have much familiarity with the code and I looked at the
other usages of TokenBuffer but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.

Unit tests: fail. 61674 tests passed, 181 failed and 781 were skipped.

failed: Clangd.Clangd/background-index.test
failed: Clangd.Clangd/code-action-request.test
failed: Clangd.Clangd/compile-commands-path-in-initialize.test
failed: Clangd.Clangd/completion-auto-trigger.test
failed: Clangd.Clangd/completion-snippets.test
failed: Clangd.Clangd/completion.test
failed: Clangd.Clangd/diagnostic-category.test
failed: Clangd.Clangd/diagnostics-no-tidy.test
failed: Clangd.Clangd/diagnostics-notes.test
failed: Clangd.Clangd/diagnostics.test
failed: Clangd.Clangd/did-change-configuration-params.test
failed: Clangd.Clangd/document-link.test
failed: Clangd.Clangd/execute-command.test
failed: Clangd.Clangd/filestatus.test
failed: Clangd.Clangd/fixits-codeaction.test
failed: Clangd.Clangd/fixits-command.test
failed: Clangd.Clangd/fixits-embed-in-diagnostic.test
failed: Clangd.Clangd/formatting.test
failed: Clangd.Clangd/hover.test
failed: Clangd.Clangd/index-tools.test
failed: Clangd.Clangd/path-mappings.test
failed: Clangd.Clangd/protocol.test
failed: Clangd.Clangd/references.test
failed: Clangd.Clangd/rename.test
failed: Clangd.Clangd/request-reply.test
failed: Clangd.Clangd/selection-range.test
failed: Clangd.Clangd/semantic-highlighting.test
failed: Clangd.Clangd/signature-help-with-offsets.test
failed: Clangd.Clangd/signature-help.test
failed: Clangd.Clangd/symbol-info.test
failed: Clangd.Clangd/symbols.test
failed: Clangd.Clangd/system-include-extractor.test
failed: Clangd.Clangd/test-uri-posix.test
failed: Clangd.Clangd/textdocument-didchange-fail.test
failed: Clangd.Clangd/trace.test
failed: Clangd.Clangd/tweaks-format.test
failed: Clangd.Clangd/type-hierarchy.test
failed: Clangd.Clangd/unsupported-method.test
failed: Clangd.Clangd/utf8.test
failed: Clangd.Clangd/xrefs.test
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.CmdLineHash
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.DirectIncludesTest
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.IndexTwoFiles
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoCrashOnErrorFile
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoDotsInAbsPath
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageEmptyFile
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageLoad
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageTest
failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.UncompilableFiles
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.CommentsFromSystemHeaders
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.DynamicIndexIncludeInsertion
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.DynamicIndexMultiFile
failed: Clangd Unit Tests._/ClangdTests/CompletionTest.UsingDecl
failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.DirtyBuffer
failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.WithUpToDateIndex
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.BasicSymbols
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.DeclarationDefinition
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Enums
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.ExternSymbol
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FromMacro
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FuncTemplates
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.InHeaderFile
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Namespaces
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.NoLocals
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Qualifiers
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.QualifiersWithTemplateArgs
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.TempSpecs
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Template
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Unnamed
failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.UsingDirectives
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.ClassMembers
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CollectMacros
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CustomizedURIScheme
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.HasSystemHeaderMappingsInPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IncludeCollected
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IndexAST
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IndexMultiASTAndDeduplicate
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MacroRefs
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MergeMainFileSymbols
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.NoLocal
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.RebuildWithPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.ReferencesInMainFileWithPreamble
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.Refs
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.Relations
failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.TemplateParamsInLabel
failed: Clangd Unit Tests._/ClangdTests/FindReferences.NeedsIndexForMacro
failed: Clangd Unit Tests._/ClangdTests/FindReferences.NeedsIndexForSymbols
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.ClangdServerIntegration
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.FromHeaderToSource
failed: Clangd Unit Tests._/ClangdTests/HeaderSourceSwitchTest.FromSourceToHeader
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.CollectIncludeGraph
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphDynamicInclude
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphSelfInclude
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.IncludeGraphSkippedFile
failed: Clangd Unit Tests._/ClangdTests/IndexActionTest.NoWarnings
failed: Clangd Unit Tests._/ClangdTests/LocateSymbol.WithIndex
failed: Clangd Unit Tests._/ClangdTests/LocateSymbol.WithIndexPreferredLocation
failed: Clangd Unit Tests._/ClangdTests/MergeIndexTest.Refs
failed: Clangd Unit Tests._/ClangdTests/QualityTests.ConstructorDestructor
failed: Clangd Unit Tests._/ClangdTests/QualityTests.IsInstanceMember
failed: Clangd Unit Tests._/ClangdTests/QualityTests.NoBoostForClassConstructor
failed: Clangd Unit Tests._/ClangdTests/QualityTests.SymbolQualitySignalExtraction
failed: Clangd Unit Tests._/ClangdTests/QualityTests.SymbolRelevanceSignalExtraction
failed: Clangd Unit Tests._/ClangdTests/RenameTest.Renameable
failed: Clangd Unit Tests._/ClangdTests/SignatureHelpTest.DynamicIndexDocumentation
failed: Clangd Unit Tests._/ClangdTests/Subtypes.ClassTemplate
failed: Clangd Unit Tests._/ClangdTests/Subtypes.DependentBase
failed: Clangd Unit Tests._/ClangdTests/Subtypes.LazyResolution
failed: Clangd Unit Tests._/ClangdTests/Subtypes.MultipleInheritance
failed: Clangd Unit Tests._/ClangdTests/Subtypes.SimpleInheritance
failed: Clangd Unit Tests._/ClangdTests/Subtypes.TemplateSpec1
failed: Clangd Unit Tests._/ClangdTests/Subtypes.TemplateSpec2
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.AvoidUsingFwdDeclsAsCanonicalDecls
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CBuiltins
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CanonicalSTLHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ClassForwardDeclarationIsCanonical
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ClassMembers
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CollectMacros
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CollectSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.CountReferences
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.DeprecatedSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.DoNotIndexSymbolsInFriendDecl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Documentation
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ExternC
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.FileLocal
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.HeaderAsMainFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.HeaderGuardDetected
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IWYUPragma
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IWYUPragmaWithDoubleQuotes
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ImplementationDetail
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncFileInNonHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncludeEnums
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.IncludeHeaderSameAsFileURI
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.InvalidSourceLoc
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Locations
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacroRefInHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacroRefWithoutCollectingSymbol
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MacrosWithRefFilter
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.MainFileIsHeaderWhenSkipIncFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NameReferences
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NamelessSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.NonModularHeader
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ObjCPropertyImpl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ObjCSymbols
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Origin
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.ReferencesInFriendDecl
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Refs
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.RefsInHeaders
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.RefsOnMacros
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Relations
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Scopes
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SkipIncFileWhenCanonicalizeHeaders
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SkipInlineNamespace
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Snippet
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolFormedByCLI
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolFormedFromRegisteredSchemeFromMacro
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolRelativeNoFallback
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolRelativeWithFallback
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolWithDocumentation
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.SymbolsInMainFile
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.Template
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.TemplateArgs
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UTF16Character
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UnittestURIScheme
failed: Clangd Unit Tests._/ClangdTests/SymbolCollectorTest.UsingDecl
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromImplicitSpec
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromPartialSpec
failed: Clangd Unit Tests._/ClangdTests/TypeHierarchy.DeriveFromTemplate
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.AnonymousNamespace
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Enums
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.GlobalNamespaceQueries
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Globals
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.InMainFile
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Macros
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.MultiFile
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Namespaces
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.NoLocals
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Ranking
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.TempSpecs
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.Unnamed
failed: Clangd Unit Tests._/ClangdTests/WorkspaceSymbolsTest.WithLimit

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I started using TokenBuffer, but I ran into the following issue: when I'm
creating TokenBuffer and TokenCollector, they do not contain any tokens.
Preprocessor does not seem to have a non-null Lexer instance, TokenWatcher
(set in TokenCollector) is not triggered anywhere and neither is
Lexer::Lex. I don't have much familiarity with the code and I looked at the
other usages of TokenBuffer but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.

right, I forgot that TokenCollector must be constructed before starting to process the file, and unfortunately in some
code paths we create a SymbolCollector long after parsing the file. You can make use of syntax::Tokenize for now,
as we only care about SpelledTokens in the file. Please leave a TODO though saying that, we should rather pass one from
the caller of SymbolCollector.

Sorry for the inconvenience.

clang-tools-extra/clangd/index/Ref.h
33

Do you mean that the index should be generated again after this change because it would no longer be valid? (this one I understand)

yes i meant this

kbobyrev updated this revision to Diff 239498.Jan 22 2020, 12:25 AM

Address review comments

kbobyrev marked 4 inline comments as done.Jan 22 2020, 12:26 AM
kbobyrev planned changes to this revision.Jan 22 2020, 12:30 AM

Will put a TODO and revert helpers back to use plain binary search over the tokens.

kbobyrev updated this revision to Diff 239502.Jan 22 2020, 12:42 AM

Actually, this approach might be more generic (i.e. not relying on
implementation too much). Added the FIXME.

kbobyrev updated this revision to Diff 239503.Jan 22 2020, 12:42 AM

Move added rename unit test to the end of the list.

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

the scope of this patch is not very clear, the changes touch two different code parts SymbolCollector, and Rename, and we are lacking tests for SymbolCollector. I'd suggest spliting this patch into smaller patches:

  • a patch that adds a new kind to the ref, and updates the SymbolCollector
  • a patch that updates the rename accordingly
clang-tools-extra/clangd/index/Ref.h
33

could you please add some brief documentation on these fields?

34

The All now indicates all spelled refs. I think All should include both non-spelled and spell refs, which should be declaration | Definition | Reference.

clang-tools-extra/clangd/index/SymbolCollector.cpp
579

looks like we don't use the MainFileURI variable below, I think we can remove this if statement.

586

since we only use FilesToTokensCache in this function, make it as a local variable rather than class member.

587

note that the DeclRefs may contains references from non-main files, e.g. included headers if RefsInHeaders is true. I think we need to tokenize other files if the reference is not from main file. CollectRef lambda is a better place to place the FilesToTokensCache logic.

kadircet added inline comments.Jan 22 2020, 2:05 AM
clang-tools-extra/clangd/index/Ref.h
34

I don't follow the argument here ,All is rather a bitmask that should be ORd with a RefKindSet.
It is not like we were saying only the references that are both declarations and definitions were acceptable before, we were saying All implies either a declaration, definition or a reference in here. So the logic seems to be correct.

kadircet resigned from this revision.Jan 22 2020, 2:06 AM
hokein added inline comments.Jan 22 2020, 2:58 AM
clang-tools-extra/clangd/index/Ref.h
34

ah, you are right, I misthought about that, thanks for the clarification.

kbobyrev updated this revision to Diff 239544.Jan 22 2020, 4:33 AM
kbobyrev marked 7 inline comments as done.

Address review comments, add implicit references filter for SymbolCollector,
test changes.

clang-tools-extra/clangd/index/SymbolCollector.cpp
587

note that the DeclRefs may contains references from non-main files, e.g. included headers if RefsInHeaders is true. I think we need to tokenize other files if the reference is not from main file.

Fair enough, fixed that!

CollectRef lambda is a better place to place the FilesToTokensCache logic.

I don't really agree with that. In my opinion CollectRef should remain a simple helper that simply puts pre-processed reference into the storage. Complicating it (and also making it work with both MacroRefs and DeclRefs while the token spelling check is only required for declarations looks suboptimal to me. If you really want me to do that, please let me know, but I personally think it might be better this way.

kbobyrev updated this revision to Diff 239545.Jan 22 2020, 4:36 AM

Attempt to drop collected reference before doing more computation to improve
performance.

Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

hokein added inline comments.Jan 22 2020, 7:16 AM
clang-tools-extra/clangd/index/Ref.h
32

could you confirm with it? this looks like a Foo class definition.

108

nit: I don't understand why we need this change? it seems irrelevant to the patch.

clang-tools-extra/clangd/index/SymbolCollector.cpp
178

I was confused at the first time reading the code -- it turns out we reset the flag in the caller.

Maybe adjust the function like RefKind toRefKind(index::SymbolRoleSet Roles, bool isSpelled)?

587

I don't really agree with that. In my opinion CollectRef should remain a simple helper that simply puts pre-processed reference into the storage. Complicating it (and also making it work with both MacroRefs and DeclRefs while the token spelling check is only required for declarations looks suboptimal to me. If you really want me to do that, please let me know, but I personally think it might be better this way.

SG.

clang-tools-extra/clangd/index/SymbolCollector.h
91 ↗(On Diff #239545)

what's the use case for this flag? I think we don't need it as we always want all references.

clang-tools-extra/clangd/refactor/Rename.cpp
299 ↗(On Diff #239545)

again, I would suggest doing this change in a follow-up patch.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
710

could you add a test case for class constructor/destructor to make sure the references are marked spelled?

class [[Foo]] {
   [[Foo]]();
   ~[[Foo]]();
}
718

I think we need to verify the RefKind in the test, just add a new gtest matcher IsRefKind.

nit: we can simplify the code by aggregating the above 3 EXPECT_THATs, like

EXPECT_THAT(Refs,
             UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                       HaveRanges(Header.ranges("Foo")))...);
clang/include/clang/Tooling/Syntax/Tokens.h
324 ↗(On Diff #239545)

nit: I would make this function come immediately after the above one (without a blank line) to avoid the duplicated documentations, e.g.

/// The spelled tokens that overlap or touch a spelling location Loc.
/// This always returns 0-2 tokens.
llvm::ArrayRef<syntax::Token>
spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef<syntax::Token> Tokens);
llvm::ArrayRef<syntax::Token>
spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer &Tokens);

the same below.

kbobyrev updated this revision to Diff 241993.Feb 3 2020, 1:45 AM
kbobyrev marked 14 inline comments as done.

Address a number of comments.

clang-tools-extra/clangd/index/Ref.h
32

Good point, I thought it should be both.

108

This one is required for SizeIs matcher, but I believe I could use UnorderedAre instead (to make sure there are no extra references).

clang-tools-extra/clangd/index/SymbolCollector.cpp
178

If my understanding is correct, the suggestion is to take isSpelled argument and apply the RefKind flag based on the value of the argument instead of using SymbolRole::Implicit. Is that right? In this case I would be concerned about doing because that increase the amount of code in case there is any other index provider that sets the flags itself.

What do you think?

clang/include/clang/Tooling/Syntax/Tokens.h
324 ↗(On Diff #239545)

Makes sense. I was concerned about Doxygen not generating comments for both of these functions, but I think it should be OK.

kbobyrev updated this revision to Diff 241994.Feb 3 2020, 1:47 AM

Remove unnecessary include.

Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: lld.ELF/linkerscript/filename-spec.s

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: lld.ELF/linkerscript/filename-spec.s

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hokein added a comment.Feb 5 2020, 1:28 AM

thanks, mostly good. the only concern from my side is about the SymbolRole::Implicit, I think we should get rid of it.

clang-tools-extra/clangd/index/Ref.h
53

maybe make sense to include an AST example?

class Foo { public: Foo(); };

Foo [[f]]; // there is a reference of `Foo` constructor around `f`, this is a non-spelled reference obviously.
clang-tools-extra/clangd/index/SymbolCollector.cpp
178

it is not about the amount of code, it is about layering. I think we should not use the Implicit. With the current change, the SymbolRole::Implicit comes from two sources:

  1. the flag can be set in libindex;
  2. the flag can be set in SymbolCollector (via the post-processing in finish)

for 1), the implementation of libindex is not completed, only object-c related decls get set. I think our aim is to set the Spelled flag if 2) is true.

Another way doing that would be keep the toRefKind implementation unchanged, and set the Spelled flag afterwards.

clang-tools-extra/clangd/index/SymbolLocation.h
12 ↗(On Diff #241994)

I would not do this change, it introduces a new dependency to this header.

clang-tools-extra/clangd/unittests/RenameTests.cpp
36

do we need this change in this patch?

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
726

I found the RefKindIs is hard to follow without reading its implementation, I think we can do it in another way, retrieve all spelled refs from Refs, and verify them:

EXPECT_THAT(SpelledRefs,
             UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                       AllOf(HaveRanges(Main.ranges("spelled"))), SpelledKind()));
kbobyrev updated this revision to Diff 242577.Feb 5 2020, 5:52 AM
kbobyrev marked 7 inline comments as done.

Address the comments.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

kbobyrev updated this revision to Diff 242578.Feb 5 2020, 5:54 AM

Remove an artifact.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hokein accepted this revision.Feb 5 2020, 7:47 AM

thanks, looks good, a few nits.

before landing the patch, could you please run the clangd-indexer on the llvm project and measure the before/after indexing time? to make sure we don't introduce a big latency.
you can run the command like time ./bin/clangd-indexer -format=binary -executor=all-TUs . > static-index.idx

clang-tools-extra/clangd/index/SymbolCollector.cpp
577–578

macro is tricky, macro references are not marked spelled, it is OK for now as we are not interested in them, but I think most of time they are spelled in the source code. Maybe add a comment?

592

nit: the comment seems stale now.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
723

nit: instead of checking all refs, I'd check implicit references explicitly, similar to the SpelledRefs below (just add a UnspelledSlab).

This revision is now accepted and ready to land.Feb 5 2020, 7:47 AM
kbobyrev updated this revision to Diff 242809.Feb 5 2020, 11:09 PM
kbobyrev marked 2 inline comments as done.

Address another round of comments.

kbobyrev marked an inline comment as done.Feb 5 2020, 11:18 PM
kbobyrev retitled this revision from [clangd] Add a flag for implicit references in the Index to [clangd] Add a flag for spelled references in the Index.Feb 5 2020, 11:20 PM
kbobyrev edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

Unit tests: fail. 61852 tests passed, 5 failed and 780 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.