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