Store::getNodeReference() should take Item const*

Bug #948259 reported by Paul J. Lucas
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Zorba
Fix Released
Medium
Markos Zaharioudakis

Bug Description

In the Store class, there is the function that currently has the signature:

    virtual bool getNodeReference(Item_t& result, Item* node) = 0;

Specifically, it takes an Item* (pointer to non-constant Item) because it presumably mutates the Item object if it has to assign a node reference.

However, the function signature should really be:

    virtual bool getNodeReference(Item_t& result, Item const* node) = 0;

that is, take an Item const* (pointer to constant Item). The operation is "logically const." An implementation may choose not to alter the Item at all and instead store a mapping between Items and node references elsewhere.

However, for an implementation that does store the node reference in an Item itself, that doesn't change the fact that the operation is still "logically const." The node reference data member should be made "mutable."

Having the function take an Item* as it does now means that other classes elsewhere that return an Item const* for which you want to obtain its node reference must first const_cast the pointer which is ugly.

For example, the FTToken class keeps an Item const* pointing to the Item whence the token came. There is no reason FTToken itself needs the Item to be non-const, hence it declares its data member as Item const*. However, in code elsewhere, we want to obtain the node reference for a token's Item like this:

  if ( store::Item const *const token_item = token->item() ) {
    store::Item *const temp = const_cast<store::Item*>( token_item );
    if ( GENV_STORE.getNodeReference( item, temp ) ) {
      // ...
    }
  }

The introduction of the const_cast and temp is ugly and shouldn't be necessary. It would be even more ugly to change the FTToken class to use an Item* because then everything that passes an Item* to FTToken might have to be changed also, i.e., the non-const-ness would ripple throughout the code.

Note that changing the function signature to the proposed one using const is a backwards *compatible* change because increasing "const-ness" is always so. (The reverse, however, i.e., making something that is const into non-const, is a backwards incompatible change.)

Tags: store item
Revision history for this message
Chris Hillery (ceejatec) wrote :

This change would be backwards-compatible at the source level. It would not I think be backwards-compatible at the ABI level (I'm assuming, perhaps rashly, that C++ will generally mangle "const foo" differently than "foo"). However, given that the store isn't yet a public API or ever built separately from its implementations, this point is mostly moot.

Changed in zorba:
status: New → Fix Committed
Changed in zorba:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.