Bugzilla – Full Text Bug Listing
| Summary: | Override markers in C++ editor | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Tomasz Wesolowski <kosashi> | ||||||||||||||||||||||
| Component: | cdt-editor | Assignee: | Elena Laskavaia <elaskavaia.cdt> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Anton Leherbauer <aleherb+eclipse> | ||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||
| Priority: | P3 | CC: | eclipse.sprigogin, elaskavaia.cdt, malaperle, rolando.martins, yevshif | ||||||||||||||||||||||
| Version: | 7.0 | ||||||||||||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||
| OS: | Windows 7 | ||||||||||||||||||||||||
| URL: | http://wiki.eclipse.org/CDT/C_editor_enhancements/Override_annotations_and_warnings_in_classes | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 322753 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Tomasz Wesolowski
The marker generation logic works quite nicely so far; I expect it to need some more polishing after tests with complex multiple inheritance hierarchies. I expect to have this part done this week. In the meantime: 1. Where should I submit the marker generation logic? cdt.core or cdt.ui? The same question about marker definitons in plugin.xml and icons? 2. The markers are made non-persistent (no objections here?). I'm running the generation action manually at the moment, but I want it triggered (a) when a CEditor is opened and (b) for all CEditors when a CEditor is saved, after the index gets updated. Any hints how to achieve that? You can register part listener for when editor is open. Some sort of listener for save too - don't know what kind, you can try to trace code that does build on save to see where it is attached. (In reply to comment #1) > 1. Where should I submit the marker generation logic? cdt.core or cdt.ui? The > same question about marker definitons in plugin.xml and icons? I would suggest to not create markers at all, but only editor annotations. Markers are represented in the editor as annotations, but they can exist also without markers. I don't know how this feature is implemented in JDT, but I would assume they don't use markers. > 2. The markers are made non-persistent (no objections here?). I'm running the > generation action manually at the moment, but I want it triggered (a) when a > CEditor is opened and (b) for all CEditors when a CEditor is saved, after the > index gets updated. Any hints how to achieve that? The natural trigger could be the reconciler. There is also some logic which triggers the reconciler after the indexer completes. (In reply to comment #3) > I would suggest to not create markers at all, but only editor annotations. > Markers are represented in the editor as annotations, but they can exist also > without markers. I didn't realize the difference, but yes, that sounds reasonable- we want the override gadgets to be local to an editor, so if annotations work like this, then that's what I'll use. > The natural trigger could be the reconciler. There is also some logic which > triggers the reconciler after the indexer completes. Reconciler. Thanks for the pointer, I'll have a read on that. codan adds listeners on both editor open and reconsiler. You can check the code, I think it is ui.cxx plugin somewhere around startup code. (In reply to comment #1) It makes sense to stay as close to JDT as possible. If in future JDT introduces a new nice feature related to method overrides, it will be easier to port it to CDT if the original implementation doesn't diverge too much. About JDT- note that also the method icons in outline and package explorer are also decorated with tiny "implement" and "override" icons. An override check is a bit expensive (many calls to the index; sometimes the whole multiple inheritance -tree- graph needs to be searched). An idea is to have that information stored in the PDOM: bindings have their declarations and references stored, classes have their base classes, so maybe a CPPMethod could also have its overridden method accessible via index? Then the annotation generation in editor, outline and project explorer would just need a single index resolve on every method declaration. Just an idea. Created attachment 175262 [details]
routines for virtual functions in CPPSemantics
Created attachment 175266 [details]
[WiP] prototype
This is what I've got so far.
Remarks:
* done as a separate plug-in for the time being; needs to be integrated (where? all in cdt-ui or partially in cdt-core?)
- triggered by context menu action for the time being; needs to be hooked up properly (in editor+reconciler)
- the generation logic seems to function properly, but will like some testing
* current implementation generates markers, but detection and generation are separate and we can switch to annotations if we decide on that
- 3 types of markers are generated: override, implement, shadow
- if a function has many base classes, overriding description is calculated separately for each (I think it's the proper way, see example)
- sometimes many messages exist for one method; they are all contained in one marker (might be desirable to change this?)
* shadowing messages need some thought; might be filtered somehow not to appear so often when there are many overloads?
- method signatures in messages (esp. shadowing messages) need to have the parameters appended
* need to figure out how to present override/implement/shadow indicators in outline and project explorer
* markers should allow to jump to declaration upon doubleclick?
Starred where I'd especially need some advice.
Created attachment 175268 [details]
illustration
This shows what markers are generated ATM, as well as illustrates some current limitations with shadowing markers
(In reply to comment #9) > Created an attachment (id=175266) [details] > [WiP] prototype > > This is what I've got so far. > > Remarks: > * done as a separate plug-in for the time being; needs to be integrated (where? > all in cdt-ui or partially in cdt-core?) yes > - triggered by context menu action for the time being; needs to be hooked up > properly (in editor+reconciler) please do it > - the generation logic seems to function properly, but will like some testing ok > * current implementation generates markers, but detection and generation are > separate and we can switch to annotations if we decide on that what is the advantage of using markers? > - 3 types of markers are generated: override, implement, shadow ok. Do we need shadow? Does java have it? It looks like more codan error for me > - if a function has many base classes, overriding description is calculated > separately for each (I think it's the proper way, see example) don't know > - sometimes many messages exist for one method; they are all contained in one > marker (might be desirable to change this?) What are "messages"? > * shadowing messages need some thought; might be filtered somehow not to appear see comment about codan > so often when there are many overloads? > - method signatures in messages (esp. shadowing messages) need to have the > parameters appended > * need to figure out how to present override/implement/shadow indicators in > outline and project explorer how java does it? > * markers should allow to jump to declaration upon doubleclick? markers? Or annotations? markers should not be visible. Should be same way as in java. > > Starred where I'd especially need some advice. (In reply to comment #11) > > all in cdt-ui or partially in cdt-core?) > yes Sorry, I can't understand- yes to which one? > > * current implementation generates markers, but detection and generation are > > separate and we can switch to annotations if we decide on that > what is the advantage of using markers? Well, it's done and looks how it's supposed to. > > - 3 types of markers are generated: override, implement, shadow > ok. Do we need shadow? Does java have it? It looks like more codan error for me In java all non-static non-private methods are `virtual` in c++ terms, so shadowing doesn't really apply. There's the concept of 'hiding' for static methods, but it's not really important. In C++ there are virtual and non-virtual methods and shadowing is rather an important phenomenon and I believe some kind of indication would be desirable. I don't think a codan error would be suitable because shadowing isn't an error, just a language feature. > > - sometimes many messages exist for one method; they are all contained in one > > marker (might be desirable to change this?) > What are "messages"? Individual indicators "overrides Foo::X" and "overrides Bar::X", etc, there can be several of those in many place because of multiple inheritance. > > * shadowing messages need some thought; might be filtered somehow not to appear Having considered that, I'd stay with generating shadowing markers only for the same overload. This would make them appear only where they're actually important. > > * need to figure out how to present override/implement/shadow indicators in > > outline and project explorer > how java does it? I'll need to see in JDT source apparently > > * markers should allow to jump to declaration upon doubleclick? > markers? Or annotations? markers should not be visible. Should be same way as > in java. What do you mean "markers should not be visible"? With the prototype attached markers are generated, and they are visible on left bar, but not in the text, if that's what you're referring to. JDT implements org.eclipse.jface.viewers.ILightweightLabelDecorator to decorate outline and package explorer. Seems like the logic is called separately for editor and outline. I'll stay with this approach and hope it won't be so slow for C++. If we run into any problems with performance, we can thing about persisting some info about methods in the index. JDT actually has a special cache: org.eclipse.jdt.internal.corext.util.SuperTypeHierarchyCache which is used, among others, by OverrideIndicatorLabelDecorator responsible for decorating package explorer and outline. (This cache is NOT used for editor annotations.) Implementing C++ label decorations without such cache could be unscalable. Anyway, I'm going to finish the editor annotations first. Created attachment 175650 [details]
routines for virtual functions in CPPSemantics
fixed impure patch
Created attachment 175653 [details]
implementation
Moved to cdt.ui, adapted to be more like JDT and made to be triggered by reconciler.
Works quite neatly, could use some real-world testing
Created attachment 175654 [details]
icons
Hi Tomasz, it looks very promising! I just tried your latest set of patches.
Somehow, I got an NPE, I'm not sure why.
java.lang.NullPointerException
at org.eclipse.cdt.internal.ui.editor.OverrideIndicatorManager.reconciled(OverrideIndicatorManager.java:354)
at org.eclipse.cdt.internal.ui.editor.CEditor.reconciled(CEditor.java:2986)
Also, I had an infinite recursion:
CPPSemantics.isOverrideCandidate(CPPSemantics.java:3414)
CPPSemantics.methodIsReallyVirtual(CPPSemantics.java:3461)
CPPSemantics.isOverrideCandidate(CPPSemantics.java:3414)
CPPSemantics.methodIsReallyVirtual(CPPSemantics.java:3461)
...
#ifndef TESTCLASS1_H_
#define TESTCLASS1_H_
#include "TestClass2.h"
class TestClass1 : public TestClass2
{
public:
virtual ~TestClass1();
};
#endif
#ifndef TESTCLASS2_H_
#define TESTCLASS2_H_
#include "TestClass1.h"
class TestClass2 : public TestClass1 {
public:
virtual ~TestClass2();
};
#endif
Also, do you plan to add the markers to method definition? That would be great!
(In reply to comment #18) > Hi Tomasz, it looks very promising! Glad to hear that! Thanks for the quick feedback. > Somehow, I got an NPE, I'm not sure why. Apparently CReconcilingStrategy.reconcile(boolean) may sometimes run a ICReconcilingListener passing null AST. I made the indicator manager ignore such cases. > Also, I had an infinite recursion: I didn't think of that :) Fixed. > Also, do you plan to add the markers to method definition? That would be great! Good idea- done. I'll submit those fixes together when I'm done with opening declaration via marker. (In reply to comment #15) > Created an attachment (id=175650) [details] > routines for virtual functions in CPPSemantics > fixed impure patch Please use the existing methods ClassTypeHelper.isVirtual(..), etc. (In reply to comment #20) > Please use the existing methods ClassTypeHelper.isVirtual(..), etc. Thanks for the pointer, I didn't know of that class. :) However, my effort on re-implementing that is not really lost as there may be some problems with implementation details in ClassTypeHelper.isVirtual and isOverrider- the most obvious is ignoring return type covariance in both functions (we cannot rely on IType.isSameType(IType) here). I guess the approach would be to abort the changes to CPPSemantics, but fix ClassTypeHelper instead. I'll have a look for more problems there and open a new bug for that. (In reply to comment #21) > (In reply to comment #20) > ... > I guess the approach would be to abort the changes to CPPSemantics, but fix > ClassTypeHelper instead. I'll have a look for more problems there and open a > new bug for that. That will be great, then the current clients can take advantage of your fixes. OK, `open super declaration` finally works (ported from JDT), but the way `annotation actions` are implemented (both in CDT in JDT) is frightening. Did you know you cannot jump to super implementation from that marker if you have other errors in the same line, be it a spelling check in a comment? I'll do some cleanup (including Markus's good remark) and I'll drop a patch tomorrow. (In reply to comment #23) > OK, `open super declaration` finally works (ported from JDT), but the way > `annotation actions` are implemented (both in CDT in JDT) is frightening. Did > you know you cannot jump to super implementation from that marker if you have > other errors in the same line, be it a spelling check in a comment? Yes, You know how much I hate it? I can imagine that :) -- Markus, I've opened bug 321617 for the ClassTypeHelper issue I mentioned. Created attachment 175799 [details]
implementation
OK, here we go. :)
(In reply to comment #26) > Created an attachment (id=175799) [details] > implementation > > OK, here we go. :) A severe problem with this implementation is that OverrideIndicator stores references to IBinding and IIndex which may be used later without proper index locking. Created attachment 176168 [details]
implementation
Thank you for pointing this out. Is persisting a CElement safe?
(In reply to comment #28) > Is persisting a CElement safe? Should be: ICElementHandle (In reply to comment #28) > Created an attachment (id=176168) [details] > implementation > > Thank you for pointing this out. Is persisting a CElement safe? Yes, ICElementHandle may be used safely without locking the index. I tried last patch. Few problems:
1) There are API errors caused by adding a constant in PreferenceConstansts class. Is is not used in the code - so I suggest to delete it
2) When I run the code I get runtime exception in
MethodDefinitionFinder
the code is like
try {
IASTFunctionDefinition definition = (IASTFunctionDefinition) declaration;
...
} catch (ClassCastException e) {
// ignore
}
You should not write code like this. You check instanceof. See "Effective Java" book.
3) It did not work on my simple example:
class a {
public:
void b() {};
};
class b: public a {
public:
void b() {};
}
Btw what is the different between shadows and overrides?
4) When I make it working by moving function out of the class, the navigation (click on annotation) to definition of overriden method did not work (mine was shadow).
(In reply to comment #31) > 1) There are API errors caused by adding a constant in PreferenceConstansts > class. Is is not used in the code - so I suggest to delete it Yes, i must've accidentally left it here (remains from another patch). > 2) When I run the code I get runtime exception in > MethodDefinitionFinder Can you provide a stack trace or reproduce steps? > 3) It did not work on my simple example: Note that you can't give a method the same name as the class (reserved for constructors). The visitor only checked declarations inside class body, not definitions- thanks for pointing that out, I fixed the behaviour. > 4) When I make it working by moving function out of the class, the navigation > (click on annotation) to definition of overriden method did not work (mine was > shadow). IndexUI.findAnyDeclaration was the reason, it seems not to consider definitions. Added fallback > Btw what is the different between shadows and overrides? Try such code: #include <iostream> using namespace std; struct A { virtual const char* a() { return "A\n"; } const char* b() { return "A\n"; } /* non-virtual*/ virtual const char* c() const { return "A\n"; } }; struct B : A { const char* a() { return "B\n"; } // override const char* b() { return "B\n"; } // shadow const char* c() { return "B\n"; } // shadow }; int main() { B b; A* a = &b; cout << a->a(); // B::a called cout << a->b(); // A::b called cout << a->c(); // A::c callled } Created attachment 176325 [details]
implementation
You need another instanceof check
if (declaration instanceof IASTFunctionDefinition) {
method = (ICPPMethod) getDefinitionBinding((IASTFunctionDefinition) declaration);
this would throw CCE is binding is not cpp method.
On this code
class c1 {
public:
void bar() {}; // why annotation here??
};
class c2: public c1 {
public:
void c2::bar() {}; // correct
};
It generates shadow annotation for c1::bar. Why? If I remove c2:: prefix in
the c2 class from bar - it will not be there.
In this example nothing is generated at all
class c1 {
public:
void bar();
};
class c2: public c1 {
public:
void c2::bar() {};
};
(In reply to comment #34) > You need another instanceof check > if (declaration instanceof IASTFunctionDefinition) { > method = (ICPPMethod) getDefinitionBinding((IASTFunctionDefinition) > declaration); > > this would throw CCE is binding is not cpp method. I actually expected this scenario never to happen, because that visitor is only called on ICPPASTCompositeTypeSpecifier nodes and I expected all function definitions to be methods. But yes, now that I think of it, there's a case where this may not be true: struct C { friend void foo() {} // foo is a global function }; As for comment 35 and comment 36, this looks like an indexer problem to me. Debugger reveals: in MethodDeclarationFinder.visit(IASTDeclaration): declaration is "void bar() {}" in c1 } else if (declaration instanceof IASTFunctionDefinition) { binding = getDefinitionBinding((IASTFunctionDefinition) declaration); } in getDefinitionBinding: private static IBinding getDefinitionBinding(IASTFunctionDefinition definition) { IASTDeclarator declarator = ASTQueries.findInnermostDeclarator(definition.getDeclarator()); // correct - declarator of bar() in c1 return declarator.getName().resolveBinding() // here! resolveBinding resolves to c2::bar() instead of c1::bar() } Created attachment 176451 [details] implementation fix for comment 34 Did you send a bug for 34 and 35? Btw this check (binding != null && binding instanceof ICPPMethod) first condition is redundant, instanceof does not work for null. i.e. this is good enough (binding instanceof ICPPMethod) Btw I did fix it exactly like you did (besides null check) because I was going to commit prev patch, but then I found the other bug and hold off. I will test a bit more tonight. And I got CCE (before fix) without having friend method, I can send you my example later. I applied latest patch to head (CDT 8.0). This is great contribution towards feature parity with Java. Thanks. Please report any issues related to this in a separate bug. *** cdt cvs genie on behalf of elaskavaia *** Bug 320561 - Override markers in C++ editor, contribution from Tomasz Wesolowski [*] CSelectAnnotationRulerAction.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/correction/CSelectAnnotationRulerAction.java?root=Tools_Project&r1=1.3&r2=1.4 [*] CEditorMessages.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/CEditorMessages.java?root=Tools_Project&r1=1.13&r2=1.14 [*] ConstructedCEditorMessages.properties 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ConstructedCEditorMessages.properties?root=Tools_Project&r1=1.6&r2=1.7 [+] OverrideIndicatorImageProvider.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/OverrideIndicatorImageProvider.java?root=Tools_Project&revision=1.1&view=markup [+] OverrideIndicatorManager.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/OverrideIndicatorManager.java?root=Tools_Project&revision=1.1&view=markup [*] CEditor.java 1.213 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/CEditor.java?root=Tools_Project&r1=1.212&r2=1.213 [*] CEditorMessages.properties 1.59 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/CEditorMessages.properties?root=Tools_Project&r1=1.58&r2=1.59 [*] plugin.xml 1.384 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/plugin.xml?root=Tools_Project&r1=1.383&r2=1.384 [+] over_co.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/over_co.gif?root=Tools_Project&revision=1.1&view=markup [+] implm_co.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/implm_co.gif?root=Tools_Project&revision=1.1&view=markup [+] shad_co.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/shad_co.gif?root=Tools_Project&revision=1.1&view=markup [*] CPluginImages.java 1.91 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/CPluginImages.java?root=Tools_Project&r1=1.90&r2=1.91 Note that the Override Indicator feature leads to a JUnit test failure in org.eclipse.cdt.ui.tests.text.BasicCEditorTest.testNonFileEFSResource_Bug278632 The reason is that getInputCElement() returns null in this case which causes an exception in installOverrideIndicator(). (In reply to comment #42) > Note that the Override Indicator feature leads to a JUnit test failure in > org.eclipse.cdt.ui.tests.text.BasicCEditorTest.testNonFileEFSResource_Bug278632 > The reason is that getInputCElement() returns null in this case which causes an > exception in installOverrideIndicator(). That was fixed with patch in bug 323556. (In reply to comment #43) > (In reply to comment #42) > > Note that the Override Indicator feature leads to a JUnit test failure in > > org.eclipse.cdt.ui.tests.text.BasicCEditorTest.testNonFileEFSResource_Bug278632 > > The reason is that getInputCElement() returns null in this case which causes an > > exception in installOverrideIndicator(). > That was fixed with patch in bug 323556. Any change of extending the support to templates? template <class X> class A{ public: A(){} virtual ~A(){} virtual void doit(){} }; template <class X> class AA: public A<X>{ public: AA(){} virtual ~AA(){} virtual void doit(){} <--- marker missing }; |
