[PATCH] D41537: Optionally add code completion results for arrow instead of dot

[Ilya Biryukov via Phabricator via cfe-commits]

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