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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added inline comments.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:4118
+              : diag::err_typecheck_member_reference_suggestion;
+      Diag(OpLoc, DiagID) << ConvertedBaseType << IsArrow
+                          << Base->getSourceRange() << FixIt;
----------------
ilya-biryukov wrote:
> 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.
In current version of this patch it's not needed anymore. Removed.


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


https://reviews.llvm.org/D41537