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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. `tools/clang/test/CodeComplete`. This would require adding a flag to clang and patching up `PrintingCodeCompleConsumer` to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5715
   "member reference type %0 is %select{a|not a}1 pointer; did you mean to use '%select{->|.}1'?">;
+def note_typecheck_member_reference_full_suggestion : Note<
+  "member reference type %0 may %select{|not}1 be pointer; did you mean to use '%select{->|.}1'?">;
----------------
This is not used anymore, right? Maybe remove it?


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:807
+  ///      vec_ptr.^  // completion returns an item 'push_back', replacing '.'
+  ///      with '->' vec_ptr->^ // completion returns an item 'release',
+  ///      replacing '->' with '.'
----------------
NIT: reflow on this comment  broke formatting, maybe keep it unindented instead? I.e., this should give better formatting:

```
  ///      std::unique_ptr<std::vector<int>> vec_ptr;
  ///      vec_ptr.^ 
  ///      vec_ptr->^
  ///
  ///  In 'vec_ptr.^', one of completion items is 'push_back', it requires replacing '.' with '->'.
  ///  In 'vec_ptr->^', one of completion items is 'release', it requires replacing '.' with '->'.
```


================
Comment at: lib/Sema/SemaCodeComplete.cpp:3954
+                                              RecordDecl *RD,
+                                              std::vector<FixItHint> &&FixIts) {
   // Indicate that we are performing a member access, and the cv-qualifiers
----------------
Maybe pass a single fix-it here too with a more descriptive name?
It would make the code more readable.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:4022
+
+  auto DoCompletion = [&](Expr *Base, bool IsArrow, std::vector<FixItHint> &&FixIts) -> bool {
+    if (!Base)
----------------
Maybe pass a single fix-it with more descriptive name, e.g. `Optional<FixItHint> AccessOpFixIt`?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:4105
+    std::vector<FixItHint> FixIts {FixItHint::CreateReplacement(OpLoc, Corr)};
+    //FIXME: Add assert to check FixIt range requirements.
+
----------------
This FIXME should probably live in `CodeCompletionResult` constructor or `ResultBuilder::AddResult`. It's pretty obvious that this specific code does the right thing, but it may be non-obvious about more generic code that stores a `vector<FixItHint>`


================
Comment at: test/SemaCXX/member-expr.cpp:193
+    Cl0* c;
+    return c.a;  // expected-error {{member reference type 'PR15045::Cl0 *' is a pointer; did you mean to use '->'?}}
+  }
----------------
Is this still needed after we removed the diagnostics code?


================
Comment at: tools/libclang/CIndexCodeCompletion.cpp:309
+  /// before that result for the corresponding completion item.
+  std::vector<std::vector<FixItHint>> FixItsVector;
 };
----------------
yvvan wrote:
> ilya-biryukov wrote:
> > yvvan wrote:
> > > ilya-biryukov wrote:
> > > > Storing `vector<vector<>>` here makes looks like a hack. Even though it might seem more tricky, I suggest storing an opaque pointer to `vector<FixItHint>` in each `CXCompletionResult`.  Managing the lifetime of vectors in the `AllocatedCXCodeCompleteResults` seems totally fine, but there should be a way to get to the fixits in a similar way we can get to the completion string.
> > > > More concretely, I suggest the following API:
> > > > ```
> > > > // === Index.h
> > > > typedef void* CXCompletionFixIts;
> > > > typedef struct {
> > > >    // ...
> > > >    CXCompletionFixIts FixIts;
> > > > };
> > > > 
> > > > // Get the number of fix-its.
> > > > unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);
> > > > // ... Similar to getDiagnosticFixIt
> > > > CXString clang_getCompletionFixIt((CXCompletionResult *Result, unsigned fixit_index, CXSourceRange *replacement_range);
> > > > 
> > > > 
> > > > 
> > > > // === Impl.cpp
> > > > struct AllocatedCXCodeCompleteResults : public CXCodeCompleteResults {
> > > > // .....
> > > > // Pool for allocating non-empty fixit vectors in the CXCompletionResult.
> > > > std::vector<std::vector<FixItHint>> FixItsVector
> > > > };
> > > > 
> > > > unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result) {
> > > >   auto* FixIts = static_cast<std::vector<FixItHint>*>(Result->FixIts);
> > > >   if (!FixIts)
> > > >     return 0;
> > > >   return FixIts->size();
> > > > }
> > > > ```
> > > unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);
> > > 
> > > Do you mean appending CXCompletionResult struct with the "CXCompletionFixIts FixIts" member? Doesn't it break binary compatibility (changing the struct size)?
> > Ah, you're right. If libclang promises binary compatibility with binaries compiled from headers with previous versions, we're not allowed to do this change.
> > I'm not sure what's the best way to do that, probably the interface you propose does what we want with the least amount of friction.
> > I'd be fine with leaving as is, but given that `libclang` has a stable interface, I'd get more opinions from someone who owns the code before adding this. (Another reason to split this change into two?)
> > 
> > A possible alternative that won't break binary compatibility would be to change the opaque `CXCompletionString` to point into a struct that has both `CodeCompletionString` and the fixits vector, but that means finding all use-sites of `CXCompletionString` and that might be tricky.
> I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.
> 
> By all that I mean that "the interface you propose does what we want with the least amount of friction".
> I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.

Yeah, that's probably too compilcated. And it does not make sense for completion strings inside chunks to have fix-its, the fix-its only make sense for the completion item as a whole.



https://reviews.llvm.org/D41537