[PATCH] D69165: [clangd] Store Index in Tweak::Selection

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks



================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:51
+    Selection(ParsedAST &AST, unsigned RangeBegin, unsigned RangeEnd,
+              const SymbolIndex *Index);
     /// The text of the active document.
----------------
NIT: move it closer to the `ParsedAST` in constructor as well?


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:54
     llvm::StringRef Code;
+    /// The Index for handling code-base related queries.
+    const SymbolIndex *Index = nullptr;
----------------
s/code-base/codebase
or 
"code base"

This suggestion was inspired by [[ https://en.wikipedia.org/wiki/Codebase | Wikipedia ]] xD)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:71
+  // Index to be passed into Tweak::Selection.
+  const SymbolIndex *Index = nullptr;
+
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > ilya-biryukov wrote:
> > > > How is this index populated? Each test has to mock it?
> > > 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?
> > 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? 
> > 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.
> 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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69165/new/

https://reviews.llvm.org/D69165