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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
