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