Improved completion in call context to work with:
- functions
- member functions
- constructors
- new expressions
- function call expressions
- template variants of the previous
Paths
| Differential D6880
Implementation for completion in call context for C++ ClosedPublic Authored by francisco.lopes on Jan 8 2015, 10:19 AM.
Details
Summary Improved completion in call context to work with:
Diff Detail Event Timelinefrancisco.lopes retitled this revision from to Implementation for completion in call context for C++. francisco.lopes updated this object. Comment Actions Basic functor parameter completion was added. It's currently working for: struct S { void operator()(int){} }; int main() { S s; s(>|<); } but not for: struct S { void operator()(int){} }; int main() { S{}(>|<); } Comment Actions I expect to do some refactoring to reduce code duplication if possible in the following days, and maybe get the extended support for constructors, etc. The current state can be merged now without problems, since the current behavior will be the same after I do the remaining changes. francisco.lopes edited edge metadata. Comment ActionsGood amount of refactoring. Now providing extend support for function call operator. Comment Actions @klimek Hi, I'm providing differential diffs, not sure whether they should have been accumulative. I think it's in good enough shape now, I don't expect to tweak it for a while. Comment Actions As I said, we'll first need to find somebody who knows the trade-offs of that part of the code base to be able to sign off on the high level functionality change - did you already send an email with that to cfe-dev by the way? That might be the better venue for a high level discussion of the feature... Comment Actions As per conversation at cfe-dev, I've agreed it's best for now to have the original ordinary name code-completion behavior preserved while providing the new set of overload candidates completions. I had to create an additional CXCursorKind named CXCursor_OverloadCandidate, so that there's any way to discern overload candidates results. Futurely there may be an API with specific purpose of querying for overload candidates solely, I won't implement it for now. The tests that were deprecated before were added back. francisco.lopes updated this object. Comment ActionsIf I'm not mistaken I should have been using Arcanist erroneously by submitting relative diffs. This patch is being produced solely to provide an unified diff. Comment Actions General high level comment: you're using C++11 extensively, have you made sure you're only using features MSVC 2012 supports (as per the style guide: http://llvm.org/docs/CodingStandards.html)? Comment Actions Thanks for the feedback Manuel, I didn't try it in MSVC 2012, I hope it's not a hassle to compile clang in I believe the sole critical piece regarding the style guide is the As I'm unsure about this specific usage, I'll left it as is until proper 2015-01-13 17:12 GMT-02:00 Manuel Klimek <klimek@google.com>:
Comment Actions
Yeah, MSVC 2012 doesn't like copy-list-initialization either. Fixed, thanks again for the feedback.
Comment Actions
Phabricator's Differential tool doesn't display whitespace changes (I've followed the small indentation suggestion) and also it's not possible to show differences through the commits, only through the patches that are being sent. I've rebased and added a new commit, but the changeset as a whole only changes regarding the indentation. You may check what happened at https://github.com/oblitum/clang/tree/improved_argument_hints. I finding Phabricator awkward to use for these kind of modifications. Comment Actions
Notice that in Differential the new commits are being listed, it's just not possible to see what they actually do... Comment Actions Yes, phabricator doesn't support using a single branch for multiple diffs well - the best way to use it is to create feature branches that each have a single feature... Comment Actions Removing the fix that avoids AddFunctionCandidates from calling AddMethodCandidate and AddMethodTemplateCandidate (which it shoudn't) out to a different patch to be applied separately. Comment Actions Thx for your patience; just to be clear: I think this is a great patch, but I'll need to get it down into chunks I can bite off from as reviewer :)
Comment Actions
I'm unsure whether you're just generally commenting regarding the incoming questions about the patch, or whether you're requesting to cut the patch somehow for it to be commited in separate chunks. Comment Actions Handles force added overloads in similar fashion The way candidates that failed in overload resolution (for reason of too I'm really unsure about having this feature. It seems useful at the same time Comment Actions
I've also rebased the changeset over upstream since it has been some time already. Comment Actions Ok, I think the scope of the patch is now ok - going into detailed code review :)
Comment Actions Refactoring
Comment Actions
Looking elsewhere and in docs, it seems suppressDiagnostics() is better to be used since diagnostics in lookups just for completion are not expected. Comment Actions
I've tried a build without this line just to check whether I could get any effect, but no, it didn't make a difference as far as I can see, so... ¯\_(ツ)_/¯. Comment Actions This looks much simpler and easier to understand now - thanks for putting in the effort!
Comment Actions Fix issue with completing constructors for template specializations instantiated from a template The two remaining known FIXME's should be addressed in another patch I believe. I know how
Comment Actions Thanks for the detailed feedback Manuel.
klimek edited edge metadata. Comment ActionsOk, looks good now. Thanks! This is a really nice improvement.
This revision is now accepted and ready to land.Jan 21 2015, 5:13 AM
Revision Contents
Diff 18295 include/clang-c/Index.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDecl.cpp
lib/Parse/ParseExpr.cpp
lib/Parse/ParseExprCXX.cpp
lib/Sema/SemaCodeComplete.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
test/Index/code-completion.cpp
test/Index/complete-call.cpp
test/Index/complete-constructor-params.cpp
test/Index/complete-functor-call.cpp
test/Index/complete-type-factors.m
tools/libclang/CIndex.cpp
tools/libclang/CIndexCodeCompletion.cpp
|
Ok, after thinking about it for a bit, I believe we should put this into Sema (and add a comment).