| Bug #47798 | Unnecessary non-const overloaded member functions causing Java name clashes | ||
|---|---|---|---|
| Submitted: | 2 Oct 18:31 | Modified: | 8 Oct 18:11 |
| Reporter: | Martin Zaun | ||
| Status: | Closed | ||
| Category: | Server: NDBAPI | Severity: | S4 (Feature request) |
| Version: | mysql-5.1-telco-6.2 | OS: | Any |
| Assigned to: | Martin Zaun | Target Version: | mysql-5.1-telco-6.2..7.1 |
| Tags: | cleanup, ndb | ||
[2 Oct 18:32]
Martin Zaun
Some context on why the non-const semantics of NdbOperation::getBlobHandle() is different from its const sibling: Frazer Clement wrote: > The history here is : > Originally, the 'NdbRecAttr' Api required NdbOperations to be defined > over a number of calls. This meant that the user's NdbOperation* was > non-const. Each defining call they make 'changed' the state of the > NdbOperation. > The NdbRecord Api moved NdbOperation definition to a single call in > which the operation is *fully* defined. To avoid users attempting to > mix Api styles on the same objects, a const NdbOperation* is returned > from the NdbRecord operation definition calls. > This change meant that users would be working with const NdbOperation* > more often, and this motivated the (duplication) of getNdbErrorLine() > etc. However, for Blobs it's a little different. With the 'NdbRecAttr' > Api, when you first call getBlobHandle(), the NdbOperation state is > changed to accomodate your new request for a Blob handle. Subsequent > calls to operation->getBlobHandle() for the same Blob return the same > Blob Handle. With the NdbRecord API, you must have indicated that you > wanted to work with Blobs in the single API call to define the > operation, so when you call getBlobHandle() (on a const NdbOperation*), > you are asking the NdbOperation for the BlobHandle it already holds - > similar to the second case above. > So in this case, the non-const variant really is non-const, whereas the > const variant is const. > With scans, we do not enforce the constness-after-definition-call for > various reasons. As with NdbOperation, the non-const variants make > state changes, and Scan overrides to add further handling. For the > const variant, the normal implementation in NdbOperation is sufficient. > If we were refactoring, I suppose I would rewrite the internals of the > non-const variant to call the const-variant in the case where the > BlobHandle already exists inside the operation. > In summary : the const variants are not deprecated.
[2 Oct 18:34]
Martin Zaun
On attempting a refactorization of the const and non-const versions of NdbOperation::getBlobHandle(): Martin Zaun wrote: > The bodies of getBlobHandle() in NdbOperation.cpp are _literally_, down > to white space, identical -- but their behaviour is still different. > > The bodies versions delegate to an internal, const-overloaded, pair of > functions getBlobHandle() in NdbOperstionDefine.cpp, which have different > behaviour. > > So, it would be a bug to replace one function body with a delegation > call to the identical-looking other one, for the 'this' pointer is > non-const in one case and selects a different overloaded function.
[2 Oct 18:34]
Martin Zaun
Some context on the different use cases of the non-const and const versions of NdbDictionary::getColumn(): Frazer Clement wrote: > There's 2 use cases here : > 1) 'Normal' use, when you have a const NdbDictionary::Table* or const > NdbDictionary::Index*, you use the const variants to get a const > NdbDictionary::Column*. You should not be able to call mutating members > on an existing Table, Index, or Column, and constness is used to help > with this. If we returned a non-const Column* then users could muck > about with the (shared) Column object. > 2) 'DDL' use : When you are defining a table, index, or alter table > operation. You create a non-const NdbDictionary::Table, you create > NdbDictionary::Column objects and you can add them to it. You can look > them up again and mutate them further, as long as you have a non-const > ptr to the table/index. When you're happy with your definition, you ask > NdbDictionary to create it (involving messaging to the kernel etc.). If > that succeeds, you throw away your 'definition' objects, and ask for > your newly defined table/index by name. NdbDictionary returns you a > const ptr to them.
[7 Oct 21:03]
Martin Zaun
Committed the approved patch with the wrong bug id: 44798 Setting this bug to status Documenting. The commits into 6.2 .. 7.1: http://lists.mysql.com/commits/85977 http://lists.mysql.com/commits/85978 http://lists.mysql.com/commits/85979 http://lists.mysql.com/commits/85981 The pushes from 7.1 .. 6.2: http://lists.mysql.com/commits/85982 http://lists.mysql.com/commits/85983 http://lists.mysql.com/commits/85984 http://lists.mysql.com/commits/85985
[8 Oct 18:11]
Jon Stephens
Documented changes in the NDB-6.2.19, 6.3.28, and 7.0.9 changelogs as follows:
The NDB API methods Dictionary::listEvents(),
Dictionary::listIndexes(), Dictionary::listObjects(), and
NdbOperation::getErrorLine() formerly had both const and
non-const variants. The non-const versions of these methods have
been removed. In addition, the NdbOperation::getBlobHandle()
method has been re-implemented in order to provide consistent
internal semantics.
In addition, the affected versions of these methods have been flagged with appropriate
warnings in the NDB API documentation.
Closed.
Description: This is not a bug but an API cleanup and code improvement item. NDBAPI defines a few pairs of overloaded const/non-const member functions, which causes name clashes when mapped in a straight forward manner to Java. Some of the non-const functions have the exact same semantics as their const sibling. In these cases, NDBAPI applications should use the const version, which is of more general use since it can be called on both const and non-const objects. A discussion with Frazer Clement has identified these non-const overloaded functions to be unnecessary: NdbDictionary::listObjects(List &, Object::Type) NdbDictionary::listIndexes(List &, const char * tableName) NdbDictionary::listEvents(List &) and NdbOperation::getNdbErrorLine() In addition, these functions are defined by duplicating the bodies of their const siblings, which is code-inefficient and allows for potential bugs when only one of them is changed. Please, note that the following non-const overloads do not fall into this category and must remain unaffected: NdbOperation::getBlobHandle() NdbDictionary::getColumn() How to repeat: Compare the function definitions of the const with the non-const sibling. Suggested fix: Those non-const overloaded member functions identified as unnecessary should 1) be flagged by macro DOXYGEN_SHOULD_SKIP_DEPRECATED 2) not be mapped to Java by NDB JTie 3) have their function bodies replaced with a single delegation call to their const sibling to guarantee same semantics amid future changes.