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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:71
+  // Index to be passed into Tweak::Selection.
+  const SymbolIndex *Index = nullptr;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69165