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