Page MenuHomePhabricator

[clangd] Store Index in Tweak::Selection
ClosedPublic

Authored by kadircet on Oct 18 2019, 6:09 AM.

Details

Summary

Incoming define out-of-line tweak requires access to index.

This patch simply propogates the index in ClangdServer to Tweak::Selection while
passing the AST. Also updates TweakTest to accommodate this change.

Diff Detail

Event Timeline

kadircet created this revision.Oct 18 2019, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 6:09 AM

Overall LG, just a few quick questions.

clang-tools-extra/clangd/refactor/Tweak.h
67

NIT: Let's avoid mentioning ClangdServer here. ClangdServer is a "glue" that brings things like tweaks, code completion and other code together... Mentioning it here is sort of a "reverse" dependency...

68

NIT: Maybe put it right before ParsedAST?

clang-tools-extra/clangd/unittests/TweakTesting.h
72

How is this index populated? Each test has to mock it?

kadircet marked 3 inline comments as done.Oct 21 2019, 1:56 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/TweakTesting.h
72

that's what we usually do with the indexes in the rest of the testing code.(see code completion tests for an example)

we might decide to have some defaults if there are a substantial amount of tests making use of the same pattern, wdyt?

kadircet updated this revision to Diff 225837.Oct 21 2019, 1:56 AM
  • Address comments
ilya-biryukov added inline comments.Oct 21 2019, 3:04 AM
clang-tools-extra/clangd/unittests/TweakTesting.h
72

I see two potential problems:

  • Lifetime of the index. Why not make this field a unique_ptr<SymbolIndex>? Client code wouldn't need to worry about keeping the stale pointer in the Index field when exiting...
  • Ease of discovery for common patterns. Searching for a proper helper does not usually take a lot of time, but I somehow always forget what it is. Should we maybe add a comment mentioning a function that is commonly used for mocking index in tests. WDYT?
kadircet updated this revision to Diff 225850.Oct 21 2019, 3:46 AM
  • Store unique_ptr instead of a raw one.
kadircet marked an inline comment as done.Oct 21 2019, 3:46 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/TweakTesting.h
72

Lifetime of the index. Why not make this field a unique_ptr<SymbolIndex>? Client code wouldn't need to worry about keeping the stale pointer in the Index field when exiting...

Done.

Ease of discovery for common patterns. Searching for a proper helper does not usually take a lot of time, but I somehow always forget what it is. Should we maybe add a comment mentioning a function that is commonly used for mocking index in tests. WDYT?

Unfortunately there's no such common helper, every test has different requirements while creating those indexes so has a specialized way to go from a bunch of "DSL-like input" into a memindex.

ilya-biryukov accepted this revision.Oct 21 2019, 5:43 AM
ilya-biryukov marked an inline comment as done.

LGTM. Thanks

clang-tools-extra/clangd/refactor/Tweak.h
51

NIT: move it closer to the ParsedAST in constructor as well?

54

s/code-base/codebase
or
"code base"

This suggestion was inspired by Wikipedia xD)

clang-tools-extra/clangd/unittests/TweakTesting.h
72

Unfortunately there's no such common helper, every test has different requirements while creating those indexes so has a specialized way to go from a bunch of "DSL-like input" into a memindex.

Ah, this is sad... Anyway, let's figure it out when an actual test using the index will get added...

This revision is now accepted and ready to land.Oct 21 2019, 5:43 AM
kadircet updated this revision to Diff 225871.Oct 21 2019, 6:25 AM
kadircet marked 4 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.