yvvan marked 3 inline comments as done. yvvan added a comment. I have some failing tests... So I will update the diff a bit later (Friday most likely)
================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:564 + + /// \brief For this completion result correction is required. + std::vector<FixItHint> Corrections; ---------------- ilya-biryukov wrote: > yvvan wrote: > > yvvan wrote: > > > ilya-biryukov wrote: > > > > Adding some docs here would be useful. These fix-its could be > > > > interpreted too broadly at this point. > > > > I suggest the following semantics and the comment: > > > > > > > > ``` > > > > /// \brief FixIts that *must* be applied before inserting the text for > > > > the corresponding completion item. > > > > /// Completion items with non-empty fixits will not be returned by > > > > default, they should be explicitly requested by setting > > > > CompletionOptions::IncludeCorrections. > > > > /// For the editors to be able to compute position of the cursor for > > > > the completion item itself, the following conditions are guaranteed to > > > > hold for RemoveRange of the stored fixits: > > > > /// - Ranges in the fixits are guaranteed to never contain the > > > > completion point (or identifier under completion point, if any) inside > > > > them, except at the start or at the end of the range. > > > > /// - If a fixit range starts or ends with completion point (or starts > > > > or ends after the identifier under completion point), it will contain > > > > at least one character. It allows to unambiguously recompute completion > > > > point after applying the fixit. > > > > /// The intuition is that provided fixits change code around the > > > > identifier we complete, but are not allowed to touch the identifier > > > > itself or the completion point. > > > > /// One example of completion items with corrections are the ones > > > > replacing '.' with '->' and vice versa: > > > > /// std::unique_ptr<std::vector<int>> vec_ptr; > > > > /// vec_ptr.^ // completion returns an item 'push_back', > > > > replacing '.' with '->' > > > > /// vec_ptr->^ // completion returns an item 'release', replacing > > > > '->' with '.'let's put > > > > ``` > > > > > > > > Do those invariants sound reasonable? Could we add asserts that they > > > > hold when constructing the completion results? > > > Thanks! I usually feel the lack of patience when writing descriptions :) > > "Could we add asserts that they hold when constructing the completion > > results" > > > > The current way of creating them actually assures that by default. And to > > check it once again it's required to change many things. > > Starting with the fact that there is no SourceRange contains() methos or > > something similar, > > The current way of creating them actually assures that by default. And to > > check it once again it's required to change many things. > It's still nice to have a sanity check there, mostly for the sake of new > checks that are gonna get added. > But even the current one is tricky, since it may actually contain a range > that ends exactly at the cursor (when completing right after the dot/arrow > `foo->|`). > > > Starting with the fact that there is no SourceRange contains() methos or > > something similar, > Moreover, `SourceRange` doesn't have clear semantics AFAIK. It can either be > a half-open or closed range. > Let's add a FIXME, at least, that we should add those asserts later. > It's removed from CodeCompletionString therefore it's now only above the public declaration. ================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:592 + StringRef ParentName, const char *BriefComment, + std::vector<FixItHint> FixIts); ~CodeCompletionString() = default; ---------------- ilya-biryukov wrote: > We can't store fixits in `CodeCompletionString`, it should not contain source > locatins, which are only valid for the lifetime of SourceManager. > Note that CCS has a lifetime independent of both the SourceManager and the > AST. > Storing them in CodeCompletionResult should be good enough for all our > use-cases. To support that I had to make libclang calls even worse but whatever. ================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:814 + /// \brief FixIts that *must* be applied before inserting the text for the + /// corresponding completion item. Completion items with non-empty fixits will ---------------- ilya-biryukov wrote: > We shouldn't duplicate such a large comment in too many places, it would be > impossible to keep it in sync. > I would suggest only keeping it in `CodeCompletionResult` and add a reference > to it in other places. > libclang is a bit tricky, though. It is distributed without other LLVM > headers, right? I will keep the comment only here and in libclang https://reviews.llvm.org/D41537 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
