Differential D18218

Properly display argument names of template functions
ClosedPublic
Actions

Authored by thomassc on Jan 12 2019, 9:48 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Commits
R32:49d503f8b9c1: Properly display argument names of template functions
Summary

When hovering over a C++ template function, the function argument names were shown incorrectly. For example, for the following function definition:

template <int a>
void foo(char b, char c) {}

The function would be displayed like this in the tooltip:

void foo(char a, char b)

This is because argument names, different from argument types, are fetched from argumentContext->localDeclarations() for display.
In case of template functions, both the template arguments and the function arguments are in this list, which was not accounted for.
This diff changes the behavior to count the function arguments from the end of the local declarations instead of the start, which fixes the bug.
(Note: I am not sure whether it is possible that the local declarations list also contains other entries. I did not observe
this during some short testing. Also, not sure what the situation is for other languages than C++.)

Note to reviewers: Before realizing that the issue can be solved as easily as in this diff, I implemented a solution
which associates template function parameters with that function, in the same way that template class parameters are
associated with that class (in the Identifier class). I could also post this if you think it is useful.

Test Plan

Some manual testing.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thomassc created this revision.Jan 12 2019, 9:48 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJan 12 2019, 9:48 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
thomassc requested review of this revision.Jan 12 2019, 9:48 PM
thomassc edited the summary of this revision. (Show Details)Jan 13 2019, 4:24 PM
mwolff accepted this revision.Jan 15 2019, 1:55 PM
mwolff added a subscriber: mwolff.

so this patch fixes the tooltip to show up like void foo(char b, char c), but we still don't show the template<int a> information anywhere? I guess it's a good step forward, but we should also show the template args somewhere!

This revision is now accepted and ready to land.Jan 15 2019, 1:55 PM
mwolff added a comment.Jan 15 2019, 1:55 PM

btw - do you have commit rights? if not, then feel free to request them, see https://community.kde.org/Infrastructure/Get_a_Developer_Account

you are doing really good work here and in the other changes!

aacid added a subscriber: aacid.Jan 16 2019, 10:54 PM

@mwolff he doesn't want me to commit it meanwhile or do we wait for him to get the account?

thomassc added a comment.Jan 20 2019, 3:30 PM

Yes, the behavior of this patch is as described by Milian.

Regarding commit rights, I applied for them now, but feel free to commit directly in case you want to have it in @aacid .

Regarding showing the template arguments in the tooltip as well, KDevelop's behavior seems a bit inconsistent at the moment. If you have for example this template class:

template <int A>
class TemplateClass {
  int SomeFunction() {
    return 0;
  }
};

Hovering over "TemplateClass" will not show any template arguments. However, hovering over "SomeFunction" will show "Container: TemplateClass<A>" in the tooltip. This is because the template identifier info is explicitly removed from the main tooltip display. But even if disabling this, it will only show the names of the template arguments, not the types. So, I might look into this when I find the time for it.

thomassc added a comment.Jan 20 2019, 6:07 PM

I just noticed this old bug: https://bugs.kde.org/show_bug.cgi?id=368460
Should the template parameters be put into a separate context?

Closed by commit R32:49d503f8b9c1: Properly display argument names of template functions (authored by thomassc, committed by mwolff). · Explain WhyJan 21 2019, 4:42 PM
This revision was automatically updated to reflect the committed changes.
mwolff added a comment.Jan 26 2019, 11:01 PM

@thomassc: A separate context could help, yes. But if you find alternative ways to handle it, like proposed here, then not using a separate context could potentially work out too. But it feels a bit hackish to handle the template args specially everywhere instead of putting them into a proper context.

Note that we currently don't show the template args in:

  • code completion signature
  • outline/quickopen signature
  • navigation tooltip

all of that should be fixed, and I believe a separate context would be the best way to do that

Revision Contents
Changeset List

Diff 50000

View Options

kdevplatform/language/duchain/navigation/abstractdeclarationnavigationcontext.cpp

Loading...