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_begi n;

for (++it; it != &badness.m_e nd; ++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

[Pete Becker]

Gavin Deane wrote:

Pete Becker wrote:

>Gavin Deane wrote:

>>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.

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]

On Dec 10, 5:46 am, Pete Becker <p...@versatile coding.comwrote :

Glen Dayton wrote:

Pete Becker wrote:

Glen Dayton wrote:

>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.

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 hyperventilatin g 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 'hyperventilati ng'.

[Glen Dayton]

Salt_Peter wrote:

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_begi n;

for (++it; it != &badness.m_e nd; ++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_iterat or const_baditerat or;

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::ostream&
operator<<(std: :ostream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin( ),
r_bad.vs.end(),
std::ostream_it erator< std::string >(os, "\n") );
return os;
}
};

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

for (Badness::badit erator 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<whi ch 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::param s[m_dex]; }

std::string* operator->()
{ return &(m_base->*Sample::param s[m_dex]); }
iterator& operator++()
{ ++m_dex; return *this; }

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

bool operator!=(cons t 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:: params)/sizeof(Sample:: params[0])); }

};
....

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

Glen Dayton

[Salt_Peter]

Glen Dayton wrote:

Salt_Peter wrote:

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_begi n;

for (++it; it != &badness.m_e nd; ++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_iterat or const_baditerat or;

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::ostream&
operator<<(std: :ostream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin( ),
r_bad.vs.end(),
std::ostream_it erator< std::string >(os, "\n") );
return os;
}
};

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

for (Badness::badit erator 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<whi ch 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::param s[m_dex]; }

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

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

{ return m_base->params[m_dex]; }

>

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

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

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

Sample();

Sample(std::str ing 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:: params)/sizeof(Sample:: params[0])); }

};

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

...

for (Sample::iterat or 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::iterat or iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

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

// print the original sample strings - sample
for (Sample::iterat or 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]

Salt_Peter wrote:

Glen Dayton wrote:

>Salt_Peter wrote:

>>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::strin g *it = &badness.m_begi n;

for (++it; it != &badness.m_e nd; ++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_iterat or const_baditerat or;

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::ostream&
operator<<(std: :ostream& os, const Badness& r_bad)
{
std::copy( r_bad.vs.begin( ),
r_bad.vs.end(),
std::ostream_it erator< std::string >(os, "\n") );
return os;
}
};

>Instead of using a std::vector<whi ch 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::param s[m_dex]; }

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

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

{ return m_base->params[m_dex]; }

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

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

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

Sample();

Sample(std::str ing 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: :params)/sizeof(Sample:: params[0])); }

};

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

Sorry, left off the initialization:
std::string Sample::* Sample::params[2] =
{&Sample::m_par am1, &Sample::m_para m2};

>

>...

for (Sample::iterat or 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::iterat or iter = sample.begin();
iter != sample.end();
++iter)
{
std::cout << iter->c_str() << std::endl;
}

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

// print the original sample strings - sample
for (Sample::iterat or 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
misunderstandin gs 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