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