Bugzilla – Full Text Bug Listing
| Summary: | [Generate Getters and Setters] Placing definitions in implementation file works unreliably | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc-André Laperle <malaperle> | ||||||
| Component: | cdt-refactoring | Assignee: | Sergey Prigogin <eclipse.sprigogin> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Emanuel Graf <emanuel> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | cdtdoug, eclipse.sprigogin, mschorn.eclipse | ||||||
| Version: | 8.0 | ||||||||
| Target Milestone: | 8.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
> Here, for the TestClass constructor, 'nameBinding' can be a PDOMCPPConstructor
> and 'binding' a CompositeCPPConstructor, so this is not equal.
Does CompositeCPPConstructor wrap exactly the same PDOMCPPConstructor?
Created attachment 187485 [details]
Generate Getters and Setter, CPPVisitor patch
Yes it does! How does this change look?
(In reply to comment #2) > Created attachment 187485 [details] > Generate Getters and Setter, CPPVisitor patch > > Yes it does! How does this change look? An alternative would be to change PDOMNode.equals to: public final boolean equals(Object obj) { if (obj instanceof CompositeIndexBinding) { obj = ((CompositeIndexBinding) obj).getRawBinding(); } if (obj == this) return true; if (obj instanceof PDOMNode) { PDOMNode other = (PDOMNode)obj; return getPDOM() == other.getPDOM() && record == other.record; } return super.equals(obj); } This would restore reflectivity between CompositeIndexBinding.equals and PDOMNode.equals. Markus, what is your opinion on this? In fact, we absolutely have to change either PDOMNode.equals or CompositeIndexBinding.equals because currently the following fundamental principle is violated: Symmetric: For any non-null reference values x and y, x.equals(y) must return true if and only if y.equals(x) returns true. Comment on attachment 187485 [details]
Generate Getters and Setter, CPPVisitor patch
Nice find, I think your suggested fix makes a lot of sense then!
*** cdt cvs genie on behalf of sprigogin *** Bug 335259 - Restored symmetry between PDOMNode.equals and CompositeIndexBinding.equals. [*] PDOMNode.java 1.30 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMNode.java?root=Tools_Project&r1=1.29&r2=1.30 I'm marking this as fixed. Marc-Andre, please reopen if it's not the case. (In reply to comment #3) > (In reply to comment #2) > > Created attachment 187485 [details] [details] > > Generate Getters and Setter, CPPVisitor patch > > > > Yes it does! How does this change look? > An alternative would be to change PDOMNode.equals to: > public final boolean equals(Object obj) { > if (obj instanceof CompositeIndexBinding) { > obj = ((CompositeIndexBinding) obj).getRawBinding(); > } > if (obj == this) > return true; > if (obj instanceof PDOMNode) { > PDOMNode other = (PDOMNode)obj; > return getPDOM() == other.getPDOM() && record == other.record; > } > return super.equals(obj); > } > This would restore reflectivity between CompositeIndexBinding.equals and > PDOMNode.equals. > Markus, what is your opinion on this? This implementation introduces unclear semantics, because in general it does not determine whether a PDOMBinding refers to the same entity as a CompositeIndexBinding or not. (It does not handle the case where the PDOMBinding in the CompositeIndexBinding is taken from a different PDOM) In genreal the follwoing still holds: Two bindings that are neither from the same index nor from the same AST are not equal. Consequently it'd be better to change the implementation of CompositeIndexBinding.equals() to restore the symmetric behavior (it was always reflexive). Because you cannot use equals() to determine whether bindings from different indexes refer to the same entity, the current fix is likely to be incomplete, A clean fix needs to make sure that it does not compare bindings from different indexes using equals. This raises the question, why are there different indexes in use? If this can be justified, then to compare two bindings from two different indexes or a binding from the AST and an index, you need to adapt the one binding to the index of the other (IIndex.adaptBinding(..)). I reopen the bug for reconsideration. (In reply to comment #8) > In genreal the follwoing still holds: > Two bindings that are neither from the same index nor from the same AST are > not equal. I'm confused. Isn't this guaranteed by the getPDOM() == other.getPDOM() condition? I have to admit that I could never understand the purpose of composite bindings. Since every PDOM binding knows the PDOM it belongs to, can't this information be used to determine the index containing that PDOM as a fragment? The purpose of the additional field ii CompositeIndexBinding is not clear to me. > This raises the question, why are there different indexes in use? There are not. CompositeCPPConstructor is used even when there is only a single index. Markus, could you please respond to comment #9. I'd like to close this bug. (In reply to comment #9) Back than I voted against introducing composite indexes, so I am not the best person to defend them. Their purpose basically is to allow for a namespace binding that spans multiple projects (each of the PDOMs can contain different members for this binding). With that a pdom-binding can occur in the context of different indexes (differnt set of projects). To keep track of this context each and every binding needs to be wrapped. My preference back than was to provide the context (project-set) when asking for the members of a namespace. With that a PDOMBinding should never be compared with a composite binding. (Either you are working in a context with a single project --> PDOMBinding or in a context with multiple projects -> Composite Bindings). (In reply to comment #11) I'm still very confused. I understand the reason for having CompositeCPPNamespace since it contains an array of namespaces, one for each of the participating projects. What I don't understand is the need for all other composite bindings since the only extra data they have compared to their PDOM counterparts is an index reference. This index reference seems redundant since it can be derived from PDOM of the corresponding PDOM binding. It seems that when a class or a function is declared in one projects and referenced in another, it will be represented by two composite bindings that simply wrap the corresponding PDOM bindings. The wrapping seems completely redundant to me. Am I missing something? If the wrapping is in fact redundant, then it makes sense to treat a PDOM binding and its composite wrapper as the same object. (In reply to comment #12) > (In reply to comment #11) > I'm still very confused. I understand the reason for having > CompositeCPPNamespace since it contains an array of namespaces, one for each of > the participating projects. What I don't understand is the need for all other > composite bindings since the only extra data they have compared to their PDOM > counterparts is an index reference. This index reference seems redundant since > it can be derived from PDOM of the corresponding PDOM binding. No, the index cannot be derived from the PDOM (it spans multiple PDOMs). > It seems that when a class or a function is declared in one projects and > referenced in another, it will be represented by two composite bindings that > simply wrap the corresponding PDOM bindings. The wrapping seems completely > redundant to me. Am I missing something? It is almost, however you can reach namespace bindings from every other binding, eg. via IBinding.getOwner(). If you get a namespace binding as a result, then this namespace binding needs to know its context (the PDOMs that are spanned by the index). > If the wrapping is in fact redundant, then it makes sense to treat a PDOM > binding and its composite wrapper as the same object. If it'd be redundant we could remove the concept of composite bindings. The reason, why it is not, is that it specifies the context in which namespace members are looked up (and this context needs to be kept with each binding). As indicated before, the alternative is to require clients to provide this context when asking for the members of a namespace. You can find further information in bug 74433. As thinks are, you should not work with bindings that are obtained from different indexes (and if so you need to adapt one of the bindings to the index of the other). If you follow that rule a PDOMBinding will never be compared with a CompositeBinding (they are obtained from different indexes). (In reply to comment #13) > No, the index cannot be derived from the PDOM (it spans multiple PDOMs). Can a PDOM be shared between multiple indexes? If not, then the index can be uniquely determined from any PDOM it contains. (In reply to comment #14) > (In reply to comment #13) > > No, the index cannot be derived from the PDOM (it spans multiple PDOMs). > Can a PDOM be shared between multiple indexes? If not, then the index can be > uniquely determined from any PDOM it contains. Yes it can. Created attachment 190455 [details]
Fix for CompositeIndexBinding.equals method
CompositeIndexBinding.equals method is now defined as:
public boolean equals(Object other) {
if (other == this)
return true;
if (!(other instanceof CompositeIndexBinding))
return false;
CompositeIndexBinding otherComposite = (CompositeIndexBinding) other;
return rbinding.equals(otherComposite.rbinding) && cf.equals(otherComposite.cf);
}
|
8.0.0.201101231203 Using the new, faster Generate Getters and Setters, often it will not find the implementation file. I'm not sure what the exact steps are to reproduce this, but it seems like it works ok after Eclipse just started. Then I do some stuff, like open the cpp file, then go back to the header and try again the same refactoring...often it won't find the implementation file. Not sure what's wrong, but here's what I got so far... TestClass.h: #ifndef TESTCLASS_H_ #define TESTCLASS_H_ class TestClass { int a; public: TestClass(); }; #endif TestClass.cpp: #include "TestClass.h" TestClass::TestClass() { } In CPPVisitor.CollectDeclarationsAction.isDeclarationsBinding: ... if (nameBinding.equals(binding)) { return true; } Here, for the TestClass constructor, 'nameBinding' can be a PDOMCPPConstructor and 'binding' a CompositeCPPConstructor, so this is not equal. If I apply the changes in CPPVisitor from https://bugs.eclipse.org/bugs/attachment.cgi?id=186346 and changing the condition to if ((nameBinding instanceof IIndexBinding) && (binding instanceof IIndexBinding) then it works. Not sure if that's really the solution...