| Summary: | Generate getters and setters handles arrays incorrectly | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Tomasz Wesolowski <kosashi> |
| Component: | cdt-refactoring | Assignee: | Sergey Prigogin <eclipse.sprigogin> |
| Status: | RESOLVED FIXED | QA Contact: | Sergey Prigogin <eclipse.sprigogin> |
| Severity: | normal | ||
| Priority: | P3 | CC: | eclipse.sprigogin, yevshif |
| Version: | 7.0 | ||
| Target Milestone: | 8.1.0 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
|
Description
Tomasz Wesolowski
Or maybe create a per-element getters and setters instead of per-whole-array getters and setters?
class X {
float foo[3];
public:
// generated:
void setFoo(float foo, int idx);
float getFoo(int idx);
};
Fixed by disallowing setter and generating getter that returns a const pointer. *** cdt git genie on behalf of Sergey Prigogin ***
Bug 319278 - Generate getters and setters handles arrays incorrectly.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=5ee7759f72b9c168e9446d77c8718c89bb71e372
(In reply to comment #2) > Fixed by disallowing setter and generating getter that returns a const pointer. If there's no setter, then the getter should probably return a non-const pointer to allow modification? (A const-getter returning a const pointer should be available too in that case.) (In reply to comment #4) > (In reply to comment #2) > > Fixed by disallowing setter and generating getter that returns a const pointer. > > If there's no setter, then the getter should probably return a non-const > pointer to allow modification? (A const-getter returning a const pointer should > be available too in that case.) This kind of promiscuous getter is a bad coding practice. I don't think we should encourage it. (In reply to comment #5) > This kind of promiscuous getter is a bad coding practice. I don't think we > should encourage it. I fail to see your point; are you suggesting that all member arrays shall be immutable from the point of view of the interface? (In reply to comment #6) > I fail to see your point; are you suggesting that all member arrays shall be > immutable from the point of view of the interface? Returning non-const pointers or references to field of the class breaks encapsulation since it allows client code to make modifications to class field without knowledge of the class code. Users are free to break encapsulation if they so choose, but we don't have to be helping them to shoot themselves in the foot. (In reply to comment #7) > (In reply to comment #6) > > I fail to see your point; are you suggesting that all member arrays shall be > > immutable from the point of view of the interface? > > Returning non-const pointers or references to field of the class breaks > encapsulation since it allows client code to make modifications to class field > without knowledge of the class code. Users are free to break encapsulation if > they so choose, but we don't have to be helping them to shoot themselves in the > foot. I can't agree how defining an interface could mean breaking encapsulation. There basically are two approaches to arrays: a) treat the array as a distinct object associated with composition relationship; the association itself can't be changed (i.e. no setter) but the array sub-object can be accessed and changed; in Java this would be done with an array with getter, in C++ equivalent implementation is a getter returning a reference to array (to retain the length in the type). b) treat the array as part of the object's state (not as a distinct object) and present a custom interface for interoperating with it (getXyzLength, getXyzAt(n), setXyzAt(n) for instance). The first looks like a more natural solution to me, but I could understand if someone preferred the latter. Both are valid OOP. (In reply to comment #8) > a) treat the array as a distinct object associated with composition > relationship; the association itself can't be changed (i.e. no setter) but the > array sub-object can be accessed and changed; in Java this would be done with > an array with getter, in C++ equivalent implementation is a getter returning a > reference to array (to retain the length in the type). Treating the array as a separate object is a violation of encapsulation since the arrays don't have methods. This is no different than returning a non-const reference to an int field. > b) treat the array as part of the object's state (not as a distinct object) and > present a custom interface for interoperating with it (getXyzLength, > getXyzAt(n), setXyzAt(n) for instance). There is nothing wrong with this approach, but any implementation of it would have to support customization of all method names similar to how this is done for simple accessors. (In reply to comment #9) > (In reply to comment #8) > > a) treat the array as a distinct object associated with composition > > relationship; the association itself can't be changed (i.e. no setter) but the > > array sub-object can be accessed and changed; in Java this would be done with > > an array with getter, in C++ equivalent implementation is a getter returning a > > reference to array (to retain the length in the type). > > Treating the array as a separate object is a violation of encapsulation since > the arrays don't have methods. Having methods isn't necessary to have a well-defined interface for an object. Methods is just one language feature to express an object's behaviour. Built-in types have their behaviour defined by built-in operators and this is their interface in OOP terms. Consider this reasoning: Built-in arrays in C and C++ don't have methods, but std::array does. Hence returning a non-const reference to a std::array object wouldn't be violation of encapsulation under your explanation. Then- a built-in array and a std::array represent exactly the same abstraction, and all the interface of a built-in array is a proper subset of the interface of a std::array object. Hence, given two same class interfaces, one using a reference to std::array and second using a reference to a built-in array, you would end up with equivalent class design in terms of OOP, only expressed by different syntax and different language features. > This is no different than returning a non-const > reference to an int field. If you were to introduce a single class with a single private int field with getters and setters, keep an object A of this class as a sub-object of class B and return a non-const reference to that object, then again the code would be semantically equivalent. Again the value of A's integer field could be changed without B ever knowing, because A is considered a different object in this case, a different abstraction outside of B's encapsulation. The situation is the same if B returns a reference to int instead of a reference to A, only that you don't invent a new redundant type B to express a type which is already present in the language, namely int. See the pattern here? (In reply to comment #10) I'm not interested in continuing this discussion. I hope, I expressed my point of view clearly enough. (In reply to comment #11) > (In reply to comment #10) > I'm not interested in continuing this discussion. I hope, I expressed my point > of view clearly enough. I agree. Theoretical OOP aside, there remains the aspect of practicality which suggests that arrays shouldn't be handicapped in terms of getters and setters. If someone needs them, they'll expect the Generate Getters / Setters dialog to generate them and be frustrated with the need to write it by hand if there's a special case which would forbid it. |