operator[] -- returning references

C

cppaddict

Hi,

Is it considered bad form to have the subscript operator return a
const reference variable? If not, what is the proper way to do it?
My question was prompted by the code below, my problematic attempt to
implement a subscript operator that returns a const reference. The
dubious code is marked at the end.

<code>

//MyClass is a large class we don't want to copy alot
//MyClass has a "char getChar()" function
class MyClass;

class SubscriptTest {
private:
std::vector<MyClass> _vect;
public:
SubscriptTest() {};
void addObj(MyClass mc) {_vect.push_back(mc);}
const MyClass& operator[] (char ch) const {return
lookUpByChar(ch);}
};

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
for (unsigned i=0; i<_vect.size(); i++) {
if (_vect.getChar() == ch)
return i;
}

//PROBLEM IS HERE
return *(new MyClass); //return value here needed to compile
}

</code>

The problem is what to return when the lookup fails. Creating a
default value via new, as I do above, seems wrong -- it wastes memory
and requires tracking the objects created and then destroying them in
Subscript's destructor.

Another option would be to create the default value as static variable
inside the lookUpByChar function or as a member variable of
SubscriptTest, and have the function always return a reference to
that. That seems reasonable to me, but I still wasn't sure.

Does anyone have any thoughts/suggestions?

Thanks,
cpp
 
R

Rolf Magnus

cppaddict said:
Hi,

Is it considered bad form to have the subscript operator return a
const reference variable?

No, not for the const version of it. It's actually the preferred way.
If not, what is the proper way to do it?
My question was prompted by the code below, my problematic attempt to
implement a subscript operator that returns a const reference. The
dubious code is marked at the end.

<code>

//MyClass is a large class we don't want to copy alot
//MyClass has a "char getChar()" function
class MyClass;

class SubscriptTest {
private:
std::vector<MyClass> _vect;
public:
SubscriptTest() {};
void addObj(MyClass mc) {_vect.push_back(mc);}
const MyClass& operator[] (char ch) const {return
lookUpByChar(ch);}
};

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
for (unsigned i=0; i<_vect.size(); i++) {
if (_vect.getChar() == ch)
return i;
}

//PROBLEM IS HERE
return *(new MyClass); //return value here needed to compile
}

</code>

The problem is what to return when the lookup fails. Creating a
default value via new, as I do above, seems wrong -- it wastes memory
and requires tracking the objects created and then destroying them in
Subscript's destructor.

Another option would be to create the default value as static variable
inside the lookUpByChar function or as a member variable of
SubscriptTest, and have the function always return a reference to
that. That seems reasonable to me, but I still wasn't sure.

Does anyone have any thoughts/suggestions?


Throw an exception.
 
R

Rolf Magnus

cppaddict said:
Rolf,

Doesn't the compiler require a return statement no matter what?

Not if that code path can't be reached anyway. Actually, my compiler.
You could just write:

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
        for (unsigned i=0; i<_vect.size(); i++) {
                if (_vect.getChar() == ch)
                        return i;
        }
        
throw SomeException("character not found");
        return MyClass(); //shut the compiler up
}

Yes, this would mean reuturning a reference to a temporary, but since
it's never actually reached, it doesn't hurt.
 
R

Rolf Magnus

Rolf said:
Not if that code path can't be reached anyway. Actually, my compiler.

Insert "gives a warning if there is a return statement that can never be
reached" at a place of your choice.
 
D

Daniel T.

cppaddict said:
Hi,

Is it considered bad form to have the subscript operator return a
const reference variable? If not, what is the proper way to do it?
My question was prompted by the code below, my problematic attempt to
implement a subscript operator that returns a const reference. The
dubious code is marked at the end.

<code>

//MyClass is a large class we don't want to copy alot
//MyClass has a "char getChar()" function
class MyClass;

class SubscriptTest {
private:
std::vector<MyClass> _vect;
public:
SubscriptTest() {};
void addObj(MyClass mc) {_vect.push_back(mc);}
const MyClass& operator[] (char ch) const {return
lookUpByChar(ch);}
};

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
for (unsigned i=0; i<_vect.size(); i++) {
if (_vect.getChar() == ch)
return i;
}

//PROBLEM IS HERE
return *(new MyClass); //return value here needed to compile
}

</code>

The problem is what to return when the lookup fails. Creating a
default value via new, as I do above, seems wrong -- it wastes memory
and requires tracking the objects created and then destroying them in
Subscript's destructor.

Another option would be to create the default value as static variable
inside the lookUpByChar function or as a member variable of
SubscriptTest, and have the function always return a reference to
that. That seems reasonable to me, but I still wasn't sure.

Does anyone have any thoughts/suggestions?


The const version and the non-const version of op[] should do the same
thing if the item isn't in the container.

You have a few choices, declare the behavior undefined (thats how op[]
is defined in vector,) insert an element (the way op[] works in map,) or
throw an exception (like vector::at does.)

What is best for the code that uses your container?
 
A

AngleWyrm

throw SomeException("character not found");
return MyClass(); //shut the compiler up
}
Yes, this would mean reuturning a reference to a temporary, but since
it's never actually reached, it doesn't hurt.

Isn't there a #pragma to disable specific warnings for one or more
instances/lines? I've never used pragma, but this might be a good
place for it, if that's how it's used.
 
M

Maxim Yegorushkin

Rolf said:
No, not for the const version of it. It's actually the preferred way.

This is rather arguable and senseless outside a context.

There is a rule of thumb: when in doubt do what built-in types do. Consider the operator[]'s behavior applied to a pointer - it always returns a pointee type reference, no matter whether the pointer itself is cv-qualified or not.

char c(0);
char* p1 = &c; // non-const pointer, non-const pointee
char* const p2 = &c; // const pointer, non-const pointee

char& r1 = *p1;
char& r2 = *p2;
 
C

cppaddict

The const version and the non-const version of op[] should do the same
thing if the item isn't in the container.

In my case, wouldn't it violate encapsulation to have a non-const
version? Essentially, that would be returning a refernece to private
data.
You have a few choices, declare the behavior undefined (thats how op[]
is defined in vector,)

How do you declare behaviour as undefined?

Thanks,
cpp
 
C

cppaddict

This is rather arguable and senseless outside a context.
There is a rule of thumb: when in doubt do what built-in types do. Consider the operator[]'s behavior applied to a pointer - it always returns a pointee type reference, no matter whether the pointer itself is cv-qualified or not.

char c(0);
char* p1 = &c; // non-const pointer, non-const pointee
char* const p2 = &c; // const pointer, non-const pointee

char& r1 = *p1;
char& r2 = *p2;

Maxim,

It's not clear to me what point you are trying to make or what advice
you are trying to give with this example. Could you please explain?

Thanks,
cpp
 
R

Rolf Magnus

AngleWyrm said:
Isn't there a #pragma to disable specific warnings for one or more
instances/lines? I've never used pragma, but this might be a good
place for it, if that's how it's used.

There might be, but it depends entirely on the compiler. The C++
standard doesn't say anything about which pragmas exist or what they
do.
 
B

bartek

The const version and the non-const version of op[] should do the same
thing if the item isn't in the container.

In my case, wouldn't it violate encapsulation to have a non-const
version? Essentially, that would be returning a refernece to private
data.

This is a question of design, and of your goals.
Consider the std::vector container - there is a non-const operator[]
overload provided, because the role of the container is to *hold*
objects, not *hide* them.

So, if your class represents some form of a container, therefore what's
the point of not providing operator[] that returns a non-const reference?
You have a few choices, declare the behavior undefined (thats how op[]
is defined in vector,)

How do you declare behaviour as undefined?

You basically state it clearly in the documentation.
 
D

Daniel T.

The const version and the non-const version of op[] should do the same
thing if the item isn't in the container.

In my case, wouldn't it violate encapsulation to have a non-const
version? Essentially, that would be returning a refernece to private
data.[/QUOTE]

Sorry, I thought this was simply some sort of container.

(Warning, the code below uses 'compose1' which was origionally part of
the STL but never actually made it into the standard.)

The proper way to do what it is you were attemptin is:

class SubscriptTest {
static const MyClass _default; // *** notice me
typedef std::vector<MyClass> MyClassContainer;
MyClassContainer _myClasses;
public:
const MyClass& operator[](char ch) const { return lookUpByChar(ch); }
const MyClass& lookUpByChar(char ch) const {
using namespace std;
MyClassContainer::const_iterator it =
find_if(_myClasses.begin(), _myClasses.end(),
compose1(bind2nd(equal_to<char>(), ch),
mem_fun_ref(&MyClass::getChar)));
return it == _myClasses.end() ? _default : *it; // ** notice me
}
};

The idea here is that you have a static const MyClass object "_default"
that is returned whenever lookUpByChar can't find an approprate object.
It works like your origional did, except without the memory allocation
hassles.

The problem is, the client won't like the result:

void client( const SubscriptTest& st ) {
assert( st['a'].getChar() == 'a' );
}

The assert will fail if the default object is returned.

So despite the fact that it "works", it's not a good idea.

You have a few choices, declare the behavior undefined (thats how op[]
is defined in vector,)

How do you declare behaviour as undefined?

First you would need a sanity check:

// return true if a MyClass object can be
// found who's getChar() returns ch
bool SubscriptTest::hasMyClass( char ch ) const {
return find_if( _myClasses.begin(),
_myClasses.end(), compose1( bind2nd( equal_to<char>(), ch ),
mem_fun_ref(&MyClass::getChar) ) ) != _myClasses.end();
}

then:

// if 'ch' can't be found, then results are undefined
const MyClass& SubscriptTest::lookUpByChar(char ch) const {
assert( hasMyClass( ch ) ); // *** notice me
return find_if( _myClasses.begin(),
_myClasses.end(), compose1( bind2nd( equal_to<char>(), ch ),
mem_fun_ref(&MyClass::getChar) ) );
}

Here, if 'ch' isn't found, the function returns *_myClasses.end() which
is of course, undefined. Fortunatly, in debug mode we first check to
make sure that the MyClass object exists. In release, the client using
this code will just have to be careful.
 
C

cppaddict

The idea here is that you have a static const MyClass object "_default"
that is returned whenever lookUpByChar can't find an approprate object.
It works like your origional did, except without the memory allocation
hassles.

The problem is, the client won't like the result:

void client( const SubscriptTest& st ) {
assert( st['a'].getChar() == 'a' );
}

The assert will fail if the default object is returned.

So despite the fact that it "works", it's not a good idea.

Daniel,

I don't understand why the client won't like the result when the
_default object is returned. Could you explain further?

Thanks,
cpp
 
D

Dave Townsend

cppaddict said:
Hi,

Is it considered bad form to have the subscript operator return a
const reference variable? If not, what is the proper way to do it?
My question was prompted by the code below, my problematic attempt to
implement a subscript operator that returns a const reference. The
dubious code is marked at the end.

<code>

//MyClass is a large class we don't want to copy alot
//MyClass has a "char getChar()" function
class MyClass;

class SubscriptTest {
private:
std::vector<MyClass> _vect;
public:
SubscriptTest() {};
void addObj(MyClass mc) {_vect.push_back(mc);}
const MyClass& operator[] (char ch) const {return
lookUpByChar(ch);}
};

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
for (unsigned i=0; i<_vect.size(); i++) {
if (_vect.getChar() == ch)
return i;
}

//PROBLEM IS HERE
return *(new MyClass); //return value here needed to compile
}

</code>

The problem is what to return when the lookup fails. Creating a
default value via new, as I do above, seems wrong -- it wastes memory
and requires tracking the objects created and then destroying them in
Subscript's destructor.

Another option would be to create the default value as static variable
inside the lookUpByChar function or as a member variable of
SubscriptTest, and have the function always return a reference to
that. That seems reasonable to me, but I still wasn't sure.

Does anyone have any thoughts/suggestions?

Thanks,
cpp


CPP,

Returning a const reference is not only good form, but is the right thing to
do if the
circumstances demand it. In the situation where you are invoking the
subscript operator
on a const object, you might want to return a const reference. In fact, you
probably need
two implementations, non-const and const so you can modify values and just
read them
respectively.

For example, in this example, the second call to operator[] uses a const
reference since s2 is a const object.
If you step through this code in the debugger, you will see the correct flow
through both the const and non-const operator[], and the appropriate
operator[] for the
element access within the _data vectors of MyString.

#include <vector>
#include <iostream>
using namespace std;

class MyString {
public:

MyString( char * data )
{ _data.assign( data, data+strlen(data)+1); }

char& operator[](int position) { return _data[position]; }

const char& operator[](int position) const { return _data[position]; }

private:
vector<char> _data;
};


int main()
{
MyString s1 = "Foo";
cout << s1[0]; // calls non-const MyString::eek:perator[]

const MyString s2 = "Bar";
cout << s2[0]; // calls const MyString::eek:perator[]

return 0;
}

I have a second point. In your posting, you say that the Myclass objects
are expensive/large to
create and so you don't want to create them unnecessarily. If your
operator[] returns references,
then you do not have to create any new objects, you can do something like
this:


class Dictionary
{
private:
vector<MyString> _words;
public:
Dictionary() {}

~Dictionary() {}

MyString& operator[]( int i )
{

int x = 0;
for ( vector<MyString>::iterator it=_words.begin(); it!=_words.end();
++it, ++x)
{
if ( i == x)
{
return *it;
}
}
// throw - you should never reach here...
// return statement here to defeat compiler error/ warning.
return _words[0];
}
const MyString& operator[] (int i ) const
{

int x =0;
for ( vector<MyString>::const_iterator cit=_words.begin();
cit!=_words.end(); ++cit, ++x)
{
if ( i == x)
{
return *cit;
}
}
// throw - we should not ever get here if i is a valid index. ....

// return statement here to defeat compiler error/warning.
return _words[0];
}


};

this example is a bit contrived, but it meant to illustrate the point that
you can avoid the creation
of new objects by returning references to existing ones in _words vector,
either the one you
are looking for _words or a default value _words[0] in case of failure.
This satisfies the
need for the compiler to find a return through all branches of the function
and stylistic needs too.


Finally, the operator[] should throw an exception in the case of an invalid
indexing value ( well
at least in the "read" case), there is not much else it can reasonably do!
It is not a reasonable interpretation to use operator[] to do lookup of an
element, that should be another function with a different interface suitable
for the task.

dave
 
D

Daniel T.

The idea here is that you have a static const MyClass object "_default"
that is returned whenever lookUpByChar can't find an approprate object.
It works like your origional did, except without the memory allocation
hassles.

The problem is, the client won't like the result:

void client( const SubscriptTest& st ) {
assert( st['a'].getChar() == 'a' );
}

The assert will fail if the default object is returned.

So despite the fact that it "works", it's not a good idea.

Daniel,

I don't understand why the client won't like the result when the
_default object is returned. Could you explain further?[/QUOTE]

I showed why. When the client calls:

char ch = mySubscriptTest['a'].getChar();

The client has every right to expect ch to equal 'a'. Unless you can
somehow guaruntee that it will, the client will be unhappy. To make such
a guaruntee, you can't use a system where a default object is returned.
 
C

cppaddict

I showed why. When the client calls:

char ch = mySubscriptTest['a'].getChar();

The client has every right to expect ch to equal 'a'. Unless you can
somehow guaruntee that it will, the client will be unhappy. To make such
a guaruntee, you can't use a system where a default object is returned.

Ok. That makes sense. Thanks for all explanations,

cpp
 
M

Maxim Yegorushkin

cppaddict said:
This is rather arguable and senseless outside a context.

There is a rule of thumb: when in doubt do what built-in types do. Consider the operator[]'s behavior applied to a pointer - it always returns a pointee type reference, no matter whether the pointer itself is cv-qualified or not.

char c(0);
char* p1 = &c; // non-const pointer, non-const pointee
char* const p2 = &c; // const pointer, non-const pointee

char& r1 = *p1;
char& r2 = *p2;

Maxim,

It's not clear to me what point you are trying to make or what advice
you are trying to give with this example. Could you please explain?

Sorry for being not clear. My aforementioned point was an example of an argument.

I think that the return type for UDT's operator[] must be decided on a per class basis. And there is no way to tell what in general operator[] must return.

Does my point make some sence?
 
S

Siemel Naran

Rolf Magnus said:
cppaddict wrote:

You can also return a pointer, where NULL means no object to return. I
think the use of returning references and using exceptions might lead to
code that is more error-free, because people may forget to test the returned
pointer against NULL.

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
for (unsigned i=0; i<_vect.size(); i++) {
if (_vect.getChar() == ch)
return i;
}

throw SomeException("character not found");
return MyClass(); //shut the compiler up
}


I'd write that as

const MyClass& SubscriptTest::lookUpByChar(char ch) const {
unsigned i = 0;
for ( ; i<_vect.size(); i++) {
if (_vect.getChar() == ch) break;
}
if (i == _vect.size()) throw SomeException();
return _vect;
}

Yes, this would mean reuturning a reference to a temporary, but since
it's never actually reached, it doesn't hurt.

True indeed, though code like returning a reference to a temporary tends to
raise warning bells.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,482
Members
44,901
Latest member
Noble71S45

Latest Threads

Top