Traversing members of a class with a pointer

[Glen Dayton]

While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
....

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
....
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

It just looks wrong. It depends on all of the members of class
or structure to be consistent so its bad from a maintenance
standpoint. It seems evil to use a pointer beyond the specific
object for which it is defined.

Looking wrong and evil, though, are not rational arguments that
would allow me to convince my technical lead that this code
needs to be changed sooner than later.

Anyone have good talking points?

/Glen

 

[Kai-Uwe Bux]

While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

It just looks wrong. It depends on all of the members of class
or structure to be consistent so its bad from a maintenance
standpoint. It seems evil to use a pointer beyond the specific
object for which it is defined.

Looking wrong and evil, though, are not rational arguments that
would allow me to convince my technical lead that this code
needs to be changed sooner than later.

Anyone have good talking points?

What about the undefined behavior that the code has; does that count?


Best

Kai-Uwe Bux

 

[Pete Becker]

Looking wrong and evil, though, are not rational arguments that would
allow me to convince my technical lead that this code needs to be
changed sooner than later.

If it works, it works. The behavior isn't defined by the C++ standard,
but I haven't run into a compiler where it won't do just what it ooks
like it does.

--

-- Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com)
Author of "The Standard C++ Library Extensions: a Tutorial and
Reference." (www.petebecker.com/tr1book)

 

[Glen Dayton]

If it works, it works. The behavior isn't defined by the C++ standard,
but I haven't run into a compiler where it won't do just what it ooks
like it does.

That it is undefined by the standard is one problem. It means
at a future date the code will stop working. That it works is
another part of the problem, because it removes a motivation for
removing a time bomb from the code. In the real-life code from
which this example was taken, macros obscured the class
beginning and end with their m_begin and m_end guards. The code
maintainer needed to add a bool and an int field to the class,
which would've broken the code if maintainer had not first
carefully analyzed it or just had good luck in realizing what
was happening. The next programmer to touch that code may not
be so lucky or smart.

I'll search the standard for a relevant reference, then I'll
write a unit test and refactor this sucker.

Because the compiler knows all the offsets of the members, is
there any chance a near future version of C++ will offer
iterators to access the typeinfo structures and offsets of each
member of a class?

/Glen

 

[red floyd]

While looking at some old code I ran across a snippet that increments a
pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

I think if you want to avoid UB, you're toast, unless you want to use
pointers to members. Maybe something like this? Note: m_members should
probably be some sort of static.

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

typedef std::vector<std::string Badness::*> member_vec_t;
member_vec_t m_members;
Badness()
{
m_members.push_back(&Badness::m_begin);
m_members.push_back(&Badness::m_param1);
m_members.push_back(&Badness::m_param2);
m_members.push_back(&Badness::m_end);
}

};


// ...
Badness badness;
for (Badness::member_vec_t it = badness.m_members.begin();
it != badness.m_members.end();
++it)
std::cout << badness.*(*it) << std::endl;

 

[Pete Becker]

That it is undefined by the standard is one problem. It means at a
future date the code will stop working.

No, it means that at a future date the code might stop working. That's
not to say that it's a good way to write code, but hyperventilating is
not a good way to start a design.

--

-- Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com)
Author of "The Standard C++ Library Extensions: a Tutorial and
Reference." (www.petebecker.com/tr1book)

 

[Gavin Deane]

No, it means that at a future date the code might stop working. That's
not to say that it's a good way to write code, but hyperventilating is
not a good way to start a design.

I think there is quite a difference between intending to use formally
undefined behaviour in a new design and changing existing code that
works when you discover it relies on formally undefined behaviour.

Personally, for a new design my initial position would be to avoid any
code for which the behaviour is undefined. With an existing code base,
if I was satisfied that the behaviour on my particular compiler and
platform was as intended, my initial position would be if it ain't
broken, don't fix it. Of course, formally the code is broken by
definition. But in practice I would take into account whether I
expected to be changing compilers any time soon, and how much
regression test coverage I had of the code before deciding whether to
change it.

Gavin Deane

 

[Salt_Peter]

While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

It just looks wrong. It depends on all of the members of class
or structure to be consistent so its bad from a maintenance
standpoint. It seems evil to use a pointer beyond the specific
object for which it is defined.

Looking wrong and evil, though, are not rational arguments that
would allow me to convince my technical lead that this code
needs to be changed sooner than later.

Anyone have good talking points?

/Glen

Loose the pointers.
Can you not do something like this? Note: you must keep the
std::vector< std::string > container at the top of the struct in order
to guarentee that it is initialized before the references.

#include <iostream>
#include <ostream>
#include <string>
#include <vector>
#include <iterator>

struct Badness {
private:
std::vector< std::string > vs;
public:
std::string& m_begin;
std::string& m_param1;
std::string& m_param2;
std::string& m_end;
typedef std::vector< std::string >::iterator baditerator;
typedef std::vector< std::string >::const_iterator const_baditerator;

Badness() : vs(4),
m_begin(vs[0]), m_param1(vs[1]),
m_param2(vs[2]), m_end(vs[3])
{ }
baditerator begin() { return vs.begin(); }
baditerator end() { return vs.end(); }

// friend
friend std:stream&
operator<<(std:stream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin(),
r_bad.vs.end(),
std:stream_iterator< std::string >(os, "\n") );
return os;
}
};

int main()
{
Badness badness;
badness.m_begin = "string m_begin";
badness.m_param1 = "string m_param1";
badness.m_param2 = "string m_param2";
badness.m_end = "string m_end";

for (Badness::baditerator iter = badness.begin();
iter != badness.end();
++iter)
{
std::cout << *iter << std::endl;
}

std::cout << "\n using op<<...\n";
std::cout << badness;
}

 

[Pete Becker]

Of course, formally the code is broken by
definition.

For a somewhat artificial definition of "broken." If it does what it's
supposed to do most people wouldn't consider it broken.

But in practice I would take into account whether I
expected to be changing compilers any time soon, and how much
regression test coverage I had of the code before deciding whether to
change it.

Portability is not an absolute. It's about how hard it is to move to a
different platform. Code that's well formed according to the language
definition isn't automatically easier to port than code that isn't.
Compilers vary.

--

-- Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com)
Author of "The Standard C++ Library Extensions: a Tutorial and
Reference." (www.petebecker.com/tr1book)

 

[Gavin Deane]

For a somewhat artificial definition of "broken." If it does what it's
supposed to do most people wouldn't consider it broken.

It does what it's supposed to do, but how do I know that? Where is the
specification that defines what behavior that code should have?

If I am relying on undefined behaviour, the simple fact that there is
nowhere I can go to read documentation that explains what the behaviour
should be makes me uncomfortable as an engineer. Operating outside the
world of specification is moving away from sound engineering principles
and towards voodoo. I would suggest that a software engineer should
only do that with good reason.

Gavin Deane

 

[Pete Becker]

It does what it's supposed to do, but how do I know that?

You test it.

Where is the
specification that defines what behavior that code should have?
If I am relying on undefined behaviour, the simple fact that there is
nowhere I can go to read documentation that explains what the behaviour
should be makes me uncomfortable as an engineer.

That's not necessarily the case. The standard does not prohibit compiler
vendors from documenting behavior that is formally undefined. But that
aside...

Operating outside the
world of specification is moving away from sound engineering principles
and towards voodoo.

Only if you don't test that the code does what you need it to do.

I would suggest that a software engineer should
only do that with good reason.

That's a different issue.

--

-- Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com)
Author of "The Standard C++ Library Extensions: a Tutorial and
Reference." (www.petebecker.com/tr1book)

 

[ShaneG]

No, it means that at a future date the code might stop working. That's
not to say that it's a good way to write code, but hyperventilating is
not a good way to start a design.

If it works, it works, but surely that isn't the only criteria for
whether code is acceptable.
You didn't quote from this poster what I think is the most compelling
reason for modifying this code, which is that a future maintainer may
modify the class in what seems to be an innocuous manner, but breaks
the code that assumes contiguous layout. I don't think being concerned
about that merits the label 'hyperventilating'.

 

[Glen Dayton]

While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

It just looks wrong. It depends on all of the members of class
or structure to be consistent so its bad from a maintenance
standpoint. It seems evil to use a pointer beyond the specific
object for which it is defined.

Looking wrong and evil, though, are not rational arguments that
would allow me to convince my technical lead that this code
needs to be changed sooner than later.

Anyone have good talking points?

/Glen

Loose the pointers.
Can you not do something like this? Note: you must keep the
std::vector< std::string > container at the top of the struct in order
to guarentee that it is initialized before the references.

#include <iostream>
#include <ostream>
#include <string>
#include <vector>
#include <iterator>

struct Badness {
private:
std::vector< std::string > vs;
public:
std::string& m_begin;
std::string& m_param1;
std::string& m_param2;
std::string& m_end;
typedef std::vector< std::string >::iterator baditerator;
typedef std::vector< std::string >::const_iterator const_baditerator;

Badness() : vs(4),
m_begin(vs[0]), m_param1(vs[1]),
m_param2(vs[2]), m_end(vs[3])
{ }
baditerator begin() { return vs.begin(); }
baditerator end() { return vs.end(); }

// friend
friend std:stream&
operator<<(std:stream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin(),
r_bad.vs.end(),
std:stream_iterator< std::string >(os, "\n") );
return os;
}
};

int main()
{
Badness badness;
badness.m_begin = "string m_begin";
badness.m_param1 = "string m_param1";
badness.m_param2 = "string m_param2";
badness.m_end = "string m_end";

for (Badness::baditerator iter = badness.begin();
iter != badness.end();
++iter)
{
std::cout << *iter << std::endl;
}

std::cout << "\n using op<<...\n";
std::cout << badness;
}

Thanks.

Instead of using a std::vector<> which requires run-time
initialization, how about just using a static array of member
pointers? That way the initialization is done at load time and
the normal type safety applies:

struct Sample
{
std::string m_param1;
std::string m_param2;

static std::string Sample::* params[2];

// Provide a convenient means of stepping through
class iterator
{
private:
iterator() : m_dex(0), m_base(NULL) {}
unsigned int m_dex;
Sample *m_base;

public:
iterator(Sample *base) : m_dex(0), m_base(base) {}
iterator(Sample *base, unsigned int dex) : m_dex(dex),
m_base(base) {}

std::string& operator*()
{ return m_base->*Sample:arams[m_dex]; }

std::string* operator->()
{ return &(m_base->*Sample:arams[m_dex]); }


iterator& operator++()
{ ++m_dex; return *this; }

bool operator==(const iterator& other)
{ return m_base == other.m_base && m_dex == other.m_dex; }

bool operator!=(const iterator& other)
{ return m_base != other.m_base || m_dex != other.m_dex; }
};

Sample();
iterator begin() { return iterator(this); }
iterator end()
{ return iterator(this,
sizeof(Sample:arams)/sizeof(Sample:arams[0])); }

};
....

for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

Glen Dayton

 

[Salt_Peter]

While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}

It just looks wrong. It depends on all of the members of class
or structure to be consistent so its bad from a maintenance
standpoint. It seems evil to use a pointer beyond the specific
object for which it is defined.

Looking wrong and evil, though, are not rational arguments that
would allow me to convince my technical lead that this code
needs to be changed sooner than later.

Anyone have good talking points?

/Glen

Loose the pointers.
Can you not do something like this? Note: you must keep the
std::vector< std::string > container at the top of the struct in order
to guarentee that it is initialized before the references.

#include <iostream>
#include <ostream>
#include <string>
#include <vector>
#include <iterator>

struct Badness {
private:
std::vector< std::string > vs;
public:
std::string& m_begin;
std::string& m_param1;
std::string& m_param2;
std::string& m_end;
typedef std::vector< std::string >::iterator baditerator;
typedef std::vector< std::string >::const_iterator const_baditerator;

Badness() : vs(4),
m_begin(vs[0]), m_param1(vs[1]),
m_param2(vs[2]), m_end(vs[3])
{ }
baditerator begin() { return vs.begin(); }
baditerator end() { return vs.end(); }

// friend
friend std:stream&
operator<<(std:stream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin(),
r_bad.vs.end(),
std:stream_iterator< std::string >(os, "\n") );
return os;
}
};

int main()
{
Badness badness;
badness.m_begin = "string m_begin";
badness.m_param1 = "string m_param1";
badness.m_param2 = "string m_param2";
badness.m_end = "string m_end";

for (Badness::baditerator iter = badness.begin();
iter != badness.end();
++iter)
{
std::cout << *iter << std::endl;
}

std::cout << "\n using op<<...\n";
std::cout << badness;
}

Thanks.

Instead of using a std::vector<> which requires run-time
initialization, how about just using a static array of member
pointers? That way the initialization is done at load time and
the normal type safety applies:

Run-time initialization has huge advantages and a design involving
static arrays will fail.
A static is essentially a singleton but not accross compilation units
<ewww>.
C++ has a solution, however, statics should be replaced with
namespaces.

struct Sample
{
std::string m_param1;
std::string m_param2;

static std::string Sample::* params[2];

There is no this in a static variable or container, therefore:

static std::string* params[2];

Static arrays can't be initialized in ctor initialization lists and you
can't control when these are allocated. Note also, that statics are
confined to their compilation unit. So a static in that source isn't
the same static variable as in the other compilation unit. Arrays are
evil, static arrays are downright dangerous accross compilation units.
Debugging these is a nightmare.

// Provide a convenient means of stepping through
class iterator
{
private:
iterator() : m_dex(0), m_base(NULL) {}
unsigned int m_dex;
Sample *m_base;

public:
iterator(Sample *base) : m_dex(0), m_base(base) {}
iterator(Sample *base, unsigned int dex) : m_dex(dex),
m_base(base) {}

std::string& operator*()
{ return m_base->*Sample:arams[m_dex]; }

{ return *m_base->params[m_dex]; }

std::string* operator->()
{ return &(m_base->*Sample:arams[m_dex]); }

{ return m_base->params[m_dex]; }

iterator& operator++()
{ ++m_dex; return *this; }

bool operator==(const iterator& other)
{ return m_base == other.m_base && m_dex == other.m_dex; }

bool operator!=(const iterator& other)
{ return m_base != other.m_base || m_dex != other.m_dex; }
};

Sample();

Sample(std::string s1, std::string s2)
: m_param1(s1), m_param2(s2)
{
params[0] = &m_param1; // not allowed in init lists
params[1] = &m_param2;
};

iterator begin() { return iterator(this); }
iterator end()
{ return iterator(this,
sizeof(Sample:arams)/sizeof(Sample:arams[0])); }

};

// required:
std::string* Sample:arams[2];

...

for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

Glen Dayton

// Now watch how nasty this gets, not to mention insidious:

int main()
{
Sample sample("string 1", "string 2"); // sample
for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

Sample another("string 3", "string 4"); // another
for (Sample::iterator iter = another.begin();
iter != another.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

// print the original sample strings - sample
for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}
}

/*
string 1
string 2
string 3
string 4
string 3 // the array no longer points to the original strings
string 4
*/

 

[Glen Dayton]

Glen Dayton wrote:
While looking at some old code I ran across a snippet that
increments a pointer to access different members of a structure:
...

struct Badness {
std::string m_begin;
std::string m_param1;
std::string m_param2;
std::string m_end;

Badness();
};
...
Badness badness;

std::string *it = &badness.m_begin;

for (++it; it != &badness.m_end; ++it) {
std::cout << it->c_str() << std::endl;
}
Loose the pointers.
Can you not do something like this? Note: you must keep the
std::vector< std::string > container at the top of the struct in order
to guarentee that it is initialized before the references.

#include <iostream>
#include <ostream>
#include <string>
#include <vector>
#include <iterator>

struct Badness {
private:
std::vector< std::string > vs;
public:
std::string& m_begin;
std::string& m_param1;
std::string& m_param2;
std::string& m_end;
typedef std::vector< std::string >::iterator baditerator;
typedef std::vector< std::string >::const_iterator const_baditerator;

Badness() : vs(4),
m_begin(vs[0]), m_param1(vs[1]),
m_param2(vs[2]), m_end(vs[3])
{ }
baditerator begin() { return vs.begin(); }
baditerator end() { return vs.end(); }

// friend
friend std:stream&
operator<<(std:stream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin(),
r_bad.vs.end(),
std:stream_iterator< std::string >(os, "\n") );
return os;
}
};

Instead of using a std::vector<> which requires run-time
initialization, how about just using a static array of member
pointers? That way the initialization is done at load time and
the normal type safety applies:

Run-time initialization has huge advantages and a design involving
static arrays will fail.
A static is essentially a singleton but not accross compilation units
<ewww>.
C++ has a solution, however, statics should be replaced with
namespaces.

struct Sample
{
std::string m_param1;
std::string m_param2;

static std::string Sample::* params[2];

There is no this in a static variable or container, therefore:

static std::string* params[2];

Static arrays can't be initialized in ctor initialization lists and you
can't control when these are allocated. Note also, that statics are
confined to their compilation unit. So a static in that source isn't
the same static variable as in the other compilation unit. Arrays are
evil, static arrays are downright dangerous accross compilation units.
Debugging these is a nightmare.

// Provide a convenient means of stepping through
class iterator
{
private:
iterator() : m_dex(0), m_base(NULL) {}
unsigned int m_dex;
Sample *m_base;

public:
iterator(Sample *base) : m_dex(0), m_base(base) {}
iterator(Sample *base, unsigned int dex) : m_dex(dex),
m_base(base) {}

std::string& operator*()
{ return m_base->*Sample:arams[m_dex]; }

{ return *m_base->params[m_dex]; }

std::string* operator->()
{ return &(m_base->*Sample:arams[m_dex]); }

{ return m_base->params[m_dex]; }

iterator& operator++()
{ ++m_dex; return *this; }

bool operator==(const iterator& other)
{ return m_base == other.m_base && m_dex == other.m_dex; }

bool operator!=(const iterator& other)
{ return m_base != other.m_base || m_dex != other.m_dex; }
};

Sample();

Sample(std::string s1, std::string s2)
: m_param1(s1), m_param2(s2)
{
params[0] = &m_param1; // not allowed in init lists
params[1] = &m_param2;
};

Whoa! params is an array of pointers to members! You've
initialized it incorrectly. See my oops below.

iterator begin() { return iterator(this); }
iterator end()
{ return iterator(this,
sizeof(Sample:arams)/sizeof(Sample:arams[0])); }

};

// required:
std::string* Sample:arams[2];

Sorry, left off the initialization:


std::string Sample::* Sample:arams[2] =
{&Sample::m_param1, &Sample::m_param2};

// Now watch how nasty this gets, not to mention insidious:

int main()
{
Sample sample("string 1", "string 2"); // sample
for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

Sample another("string 3", "string 4"); // another
for (Sample::iterator iter = another.begin();
iter != another.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

// print the original sample strings - sample
for (Sample::iterator iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}
}

/*
string 1
string 2
string 3
string 4
string 3 // the array no longer points to the original strings
string 4
*/

Thank you for your time analyzing my little program. In the
future I will include the complete source to help prevent
misunderstandings like this.

I'm surprised your code compiled. The iterator uses the pointer
to members which must be used with a base pointer. I
specifically used pointers to members so I could have diffent
instances of the same class. Notice the ->* syntax in the iterator.

/Glen Dayton