ilya-biryukov added inline comments.
================ Comment at: include/clang-c/Index.h:5237 +/** + * \brief FixIts that *must* be applied before inserting the text for the + * corresponding completion item. Completion items with non-empty fixits will ---------------- This seems too large for a brief comment :-) Maybe break with a newline after the first sentence. ================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:564 + + /// \brief For this completion result correction is required. + std::vector<FixItHint> Corrections; ---------------- 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. ================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:564 + + /// \brief FixIts that *must* be applied before inserting the text for the + /// corresponding completion item. Completion items with non-empty fixits will ---------------- I suggest moving this doc comment to `getFixits` method, as it is the public interface. ================ Comment at: include/clang/Sema/CodeCompleteConsumer.h:592 + StringRef ParentName, const char *BriefComment, + std::vector<FixItHint> FixIts); ~CodeCompletionString() = default; ---------------- 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. ================ 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 ---------------- 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? https://reviews.llvm.org/D41537 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
