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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
