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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D41537#1097601, @yvvan wrote:

> I hope this review will be over some day :)


Sorry for keeping it that long.
FWIW, I think we keep jumping back and forth between the clang and libclang changes here.
I should've suggested splitting the change into two earlier.

I promise to promptly review the changes, let's try getting it in this week. I am very excited to have this feature!



================
Comment at: lib/Sema/SemaCodeComplete.cpp:4107
+    FixItHint FixIt = FixItHint::CreateReplacement(OpLoc, Corr);
+    //FIXME: Add assert to check FixIt range requirements.
+
----------------
This FIXME should probably go into the `CompletionResult` constructor, we want to check the invariants in a single place.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:4118
+              : diag::err_typecheck_member_reference_suggestion;
+      Diag(OpLoc, DiagID) << ConvertedBaseType << IsArrow
+                          << Base->getSourceRange() << FixIt;
----------------
Why do we need to report an extra diagnostic here in completion?
Our completion items contain all the relevant information for actually doing the change, and having different diagnostics in completion vs non-completion modes does not seem like the right way to do things.


================
Comment at: tools/libclang/CIndexCodeCompletion.cpp:309
+  /// before that result for the corresponding completion item.
+  std::vector<std::vector<FixItHint>> FixItsVector;
 };
----------------
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();
}
```


https://reviews.llvm.org/D41537