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

Ivan Donchevskii via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue May 8 04:02:04 PDT 2018


yvvan added inline comments.


================
Comment at: include/clang-c/Index.h:5220
+CINDEX_LINKAGE CXString
+clang_getCompletionCorrection(CXCompletionString completion_string,
+                              unsigned correction_index,
----------------
ilya-biryukov wrote:
> I'm a bit vary about exposing source manager and language options in the API just for the sake of corrections.
> I suggest we add an extra parameter of an existing class (`CXCodeCompleteResults`) instead and remove the corresponding helpers to get source manager and language options:
> `clang_getCompletionNumCorrections(CXCompletionString completion_string, CXCodeCompleteResults* results);`
> 
Haha, I had that interface initially but thought it's too ugly and needs to be improved :)


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:564
+
+  /// \brief For this completion result correction is required.
+  std::vector<FixItHint> Corrections;
----------------
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 :)


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:564
+
+  /// \brief For this completion result correction is required.
+  std::vector<FixItHint> Corrections;
----------------
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,


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:565
+  /// \brief For this completion result correction is required.
+  std::vector<FixItHint> Corrections;
+
----------------
ilya-biryukov wrote:
> I wonder if we should call them Fixits instead?
> To follow the pattern for diagnostics.
Ok, let's call them fixit-s


https://reviews.llvm.org/D41537





More information about the cfe-commits mailing list