Bugzilla – Full Text Bug Listing
| Summary: | Editor shows bogus Override Markers when multiple inheritance is involved | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Patrick Hofer <paedu.hofer> | ||||||||||||||
| Component: | cdt-editor | Assignee: | Nathan Ridge <zeratul976> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Anton Leherbauer <aleherb+eclipse> | ||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | aegges, cdtdoug, chrislong, eclipse.sprigogin, elaskavaia.cdt, kosashi, malaperle, marc.khouzam, mschorn.eclipse, waste.manager, yevshif, zeratul976 | ||||||||||||||
| Version: | 8.0 | ||||||||||||||||
| Target Milestone: | 8.7.0 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | All | ||||||||||||||||
| See Also: |
https://git.eclipse.org/r/42961 https://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=bb4b74b3673fefdc396709a1bfc1585512fbfe7b https://git.eclipse.org/r/43178 |
||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 195892 [details]
Possible fix/workaround
This fix solves the problems which I have observed.
It looks still a little ugly and could be cleaned up further.
I'm not sure if handleBaseClass() in testForOverride() will ever return more
than one overridden or shadowed method. I might be misunderstanding something here
but in my medium sized test projects this never happened.
I haven't had the chance to run my patch on bigger production code on my day-job
yet.
Any code snipped which results in more than one returned method would
help me understand this better and is therefore very much appreciated.
Feedback is welcome.
After debugging this one I suspect there is some room for performance improvements in the OverrideIndicatorManager. I'm not entirely convinced that we need to walk the AST more than once, although I did not dig deep enough to prove this. Next, I think some object creations could be deferred until they are needed. Torturing the GC less could make a difference since this code gets executed every few ms by the reconciler. Finally we could get a massive performance boost if we apply some caching mechanism to avoid redundant work. I'm going to file a new bug if I can find enough evidence to proof my observations. It might be better to postpone any bigger refactorings until Indigo is out of the door anyway. The next maintenance release is early enough to make this faster. I like those markers! They are really helpful. It would be sad if users are disabling the override annotations because of performance issues (refer to Bug 322753). (In reply to comment #1) > Created attachment 195892 [details] > Possible fix/workaround > > This fix solves the problems which I have observed. > > It looks still a little ugly and could be cleaned up further. > > I'm not sure if handleBaseClass() in testForOverride() will ever return more > than one overridden or shadowed method. I might be misunderstanding something > here > but in my medium sized test projects this never happened. > I haven't had the chance to run my patch on bigger production code on my > day-job > yet. > > Any code snipped which results in more than one returned method would > help me understand this better and is therefore very much appreciated. > > Feedback is welcome. I am not familiar with this code therefore I have added Tomasz (the original contributor) to CC. (In reply to comment #3) > I am not familiar with this code therefore I have added Tomasz (the original > contributor) to CC. Can we add Alena to CC too? She was involved in getting the contribution in (That was bug 320561). (In reply to comment #4) > Can we add Alena to CC too? She was involved in getting the contribution in > (That was bug 320561). Sure. It's probably easier to rewrite the algorithm that determines whether a method overrides or shadows another one, rather than fixing it. An implementation, that considers all methods that belong to the same class in one step, will be more efficient. The concept of shadowing is implemented wrong, a method hides all methods with the same name regardless of the parameter types (unless it overrides a virtual method). (In reply to comment #6) > It's probably easier to rewrite the algorithm that determines whether a method > overrides or shadows another one, rather than fixing it. Yes, and it's not only the algoritme, see comment 2. If anybody can spend enough time before Indigo's code freeze this would be perfect. If not, I would like to have the workaround in. I think this bug is serious enough. (In reply to comment #7) > Yes, and it's not only the algoritme, see comment 2. > If anybody can spend enough time before Indigo's code freeze this would be > perfect. If not, I would like to have the workaround in. I think this bug is > serious enough. Are there any test-cases around that help in determining what is the less bad version? (In reply to comment #8) > Are there any test-cases around that help in determining what is the less bad > version? I don't know. With https://bugs.eclipse.org/bugs/show_bug.cgi?id=320561#c41 there were no tests added. So I doubt there are any. (In reply to comment #6) > It's probably easier to rewrite the algorithm that determines whether a method > overrides or shadows another one, rather than fixing it. I've just started to rewrite this. Having test cases would be of tremendous help. Where do they belong to? Do we already have some infrastructure to test such annotations? I am thinking of something similar to org.eclipse.cdt.codan.core.internal.checkers.AbstractClassInstantiationCheckerTest Any help in writing the tests is welcome. Created attachment 196867 [details]
Bugfixes and performance improvements
This patch should address everything mentioned in this bug so far.
Markus, can you review this?
Did you add any junits? (In reply to comment #12) > Did you add any junits? No. I don't know where to put them. (In reply to comment #13) > Having test cases would be of tremendous help. > Where do they belong to? Do we already have some infrastructure to test such > annotations? For an example of testing annotations you can look at org.eclipse.cdt.ui.tests.text.MarkOccurrenceTest. I think the test for override indicators belongs in the same package. Created attachment 197381 [details]
Improved patch
I have refactored and simplified my last patch. Next I have realigned it with the latest version of JDT. Performance should be even better, although I do not have any numbers.
Tests are following.
Created attachment 197891 [details]
unittests
Please review. Feedback and suggestions on how to improve this are welcome.
(In reply to comment #16) > Please review. Feedback and suggestions on how to improve this are welcome. Tomasz, could you have a look on the patches? Thanks. Yup. I'll look into that this week, thanks for contributing! Created attachment 198466 [details]
unittests
Updated the test case with more tests, regarding shadowing of overloads by CV-qualifier and shadowing of virtuals by non-virtuals (and similar). That's the first thing to fix.
There are another 2 issues (handling of diamond hierarchies and unnecessary shadowing markers). I'll continue to elaborate on them.
The diamond problem
---
Consider such situation:
struct Base {
virtual ~Base();
virtual void f() = 0;
};
struct Left : virtual Base {
void f();
};
struct Right : virtual Base {
void f();
};
struct Child : Left, Right {
void f();
};
What markers should be on Child::f? Currently it's just `implements Base::f via Left`. This is true, but disregards (possibly more important) information that it also overrides Left::f and Right::f.
My approach:
I believe that it we should look at the closest ancestor for each base. So for base Left, it overrides Left::f, and for base Right, it also overrides Right::f. Hence I'd place here an Override marker with text "Overrides Left::f; Overrides Right::f".
If we take out Left::f:
From the point of view of base Left, we look further up and see that this is an implementation of Base::f. For the base Right, it's the same as earlier. Hence the message would be "Implements Base::f via Left; Overrides Right::f".
If we take out Right::f:
I'd say that a correct (most informative) approach would be to stay consistent here and write "Implements Base::f via Left; Impements Base::f via Right".
The above is more or less how it was handled in my solution from a year ago.
Note that this may get more complex, for instance:
struct Base {
virtual ~Base();
virtual void f() = 0;
};
struct Left : virtual Base {
void f();
};
struct Right : virtual Base {
void f(int); // Shadows Base::f
};
struct Child : Left, Right {
void f(); // Overrides A::f; Implements O::f via B...?; Shadows B::f ... what marker to put?
};
I get green when I try to think how this would scale to bigger hierarchies with both virtual and non-virtual inheritance. :) This needs some thinking.
(adding CC: Markus Schorn as my favourite C++ lawyer; do you have any words of enlightenment?)
(In reply to comment #19) > Updated the test case with more tests, regarding shadowing of overloads by > CV-qualifier and shadowing of virtuals by non-virtuals (and similar). That's > the first thing to fix. I will have a look at it next week. I'm currently very busy in my $DAYJOB. I'm not sure if it's related, but it's also about multiple inheritance so I thought I'd mention it: bug #351612. Patrick, Tomasz - what is the status of this bug? Are either of you planning to finish working on it? As I haven't heard back, I'm going to have a look at this myself. Created attachment 251192 [details] Patch, rebased and with test failures fixed I combined the fix and test patches into one, rebased it to apply to master, and updated the fix to make the additional tests Tomasz added in comment 19 pass. The issues Tomasz brought up in comment 20 remain to be addressed, but I don't think we should hold up the patch because of them, as it already provides a lot of value to users as is. Sergey, could you push the updated patch to Gerrit? I can't do so because I'm not the author of the patch. (In reply to Nathan Ridge from comment #25) > Sergey, could you push the updated patch to Gerrit? I can't do so because > I'm not the author of the patch. I can't push it to Gerrit on behalf of Patrick because Patrick didn't sign a CLA. Why don't you push the patch under your own name? New Gerrit change created: https://git.eclipse.org/r/42961 WARNING: this patchset contains 1015 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire (In reply to Sergey Prigogin from comment #26) > (In reply to Nathan Ridge from comment #25) > > Sergey, could you push the updated patch to Gerrit? I can't do so because > > I'm not the author of the patch. > > I can't push it to Gerrit on behalf of Patrick because Patrick didn't sign a > CLA. Why don't you push the patch under your own name? Done. It would be nice if there was a way for Patrick's name to show up in the commit log, though, as this patch reflects his hard work. (In reply to Nathan Ridge from comment #28) > It would be nice if there was a way for Patrick's name to show up in the > commit log, though, as this patch reflects his hard work. You could add the "Signed-off" thing with his name. There can be multiple of those entries in a single commit. Gerrit change https://git.eclipse.org/r/42961 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=bb4b74b3673fefdc396709a1bfc1585512fbfe7b Thank you, Nathan, Patrick and Tomasz. (In reply to Nathan Ridge from comment #25) > The issues Tomasz brought up in comment 20 remain to be addressed, but I > don't think we should hold up the patch because of them, as it already > provides a lot of value to users as is. Filed bug 461337 for this. New Gerrit change created: https://git.eclipse.org/r/43178 |
On derived classes, witch inherit from multiple base classes, bogus override markers are shown. The text shown on mouse hovering is ok, but it can show the wrong marker kind. If wrong marker kind is shown the navigation doesn't work. To reproduce the error enter the following code: #include <iostream> using namespace std; class Base1 { public: virtual void mf1 () = 0; }; class Base2 { public: virtual void mf2() = 0; virtual void mf3() { cout << "Base2::mf3()" << endl; }; void mf4() { cout << "Base2::mf4()" << endl ; }; }; class Derived: public Base1, Base2 { public: virtual void mf1() { cout << "Derived::mf1()" << endl; }; // text ok, but "shadowed" marker is shown virtual void mf2() { cout << "Derived::mf2()" << endl; }; // ok virtual void mf3() { cout << "Derived::mf3()" << endl; }; // ok void mf4() { cout << "Derived::mf4()"; }; // ok }; int main() { Derived d; d.mf1(); d.mf2(); d.mf3(); d.mf4(); return 0; } If we change the order of the base classes we get a different result: class Derived: public Base2, Base1 { // changed order here public: virtual void mf1() { cout << "Derived::mf1()" << endl; }; // ok virtual void mf2() { cout << "Derived::mf2()" << endl; }; // text ok, but "shadowed" marker is shown virtual void mf3() { cout << "Derived::mf3()" << endl; }; // text ok, but "shadowed" marker is shown void mf4() { cout << "Derived::mf4()"; }; // ok }; I think I've found the reason for this wrong behaviour and will provide a patch soon.