Axter said:
The problem with your code, is that a compiler initialize variables in
the order that the variables are listed in your class, and NOT in the
order that they're listed in the initialize list.
Yep, the compiler generally warns one about this when you do it.
fixed.
Your code is not even compilable on a standard C++ compiler.
You mean on a C++ compiler that meets the standard in a specific way.
The compiler certainly should have warned me about the return of op= at
the least. I am disappointed it didn't...that is a common mistake.
(and the mistake was mine for not telling it to)
Fixed at any rate.
Obviously I rely a bit much on the compiler to tell me when I make
common errors. One can't keep track of all the little stuff and needs
decent tools to tell you when you've made basic mistakes or omisions.
I guess you could call this using the compiler as a crutch or failure
to understand...on the other hand, I don't feel my time is well spent
worrying about such things and if the tool will do that part of the
thinking for me it gives me more brain room for solving actual
problems.
And yes, I have numerous reference books because I can't memorize the
entire standard nor the libraries I use even on a daily basis. I am
fully aware of ordering, I just assumed I would be warned if I made a
mistake in that area and didn't concern myself with it....I was wrong.
In fact I had assumed so much that I didn't even look when it became a
problem.
But at least it gives you a chance to point out beginner mistakes in my
code. Difference between us is that mine is now fixed
There's no need for an assertion in your copy constructor, because if
the count is zero, you're already going to get an assertion in the
first constructor, and more over the std::copy will not copy anything
if either row or col variables are zero.
That is noted in the code. The assertion is for the programmer.
Preconditions need to be documented. It doesn't hurt anything and an
assert is the best way to provide that documentation.
Besides, such /could/ happen and be caused by some sort of memory issue
- buffer overrun for instance. The assert would catch that rare
condition where without it there is just one more bug that could have
been caught but wasn't.
Nope, "unneccisary" assertion stays. It's good practice. It is doing
what it is supposed to, informing the programmer what the following
code assumes.
IMHO, it's a poor choice to put iterators in a class, and not have
begin and end member functions, which can easily be added with one line
of code.
Actually no they can't. What happens when you increment one? Right...
I called them iterators as in some ways they meet that definition and
there needs to be a typedef but there is one very important detail I
missed. I will fix it by calling them something else as I don't want
to do the work to implement them correctly.
Yet another reason I wouldn't even do this. Too much to account for,
too much to do...vector already exists and is a pretty darn GOOD wheel.
The resize should avoid the memset command for two reasons.
First, it assumes T is a POD type, but you have no code validating it's
a POD type.
Yes, I did make that assumption in that part of the code. That is poor
practice. Fixed.
Interestingly your code completely omits this initialization.
So not only is the above method more efficient to initialize POD type
to zero
It is actually unlikely to be any different at all...except that it
works with non-POD types.
Also, by adding the assertion for zero in the resize function, you
remove the ability for the class to clear it's data by performing a
resize(0,0).
That is ok because that would be an invalid use of that function. That
function is for resizing, not for clearing...one responsibility... I
debated having that assertion in there at all as it is not necissary
for the functioning of the code. However, I saw no major problem (just
inconveniences) with disallowing 0 bounds arrays by design as your
version does so it is something I kept. Allowing it here would be
inconsistant and invalid.
I will grant you though, the concept of not allowing 0 bounded arrays
may be flawed.
I would implement a clear() but at this point it isn't clear what that
would do. Yes, this functionality would probably be expected though
isn't actually necissary since the client can accomplish it with the
functionality that exists without breaking into the class or having to
track extra data this class should be tracking...whatever they think
clear() should do.
What constitutes a clearing would need to be defined and then it could
be implemented. It can't be a 0 bounded array though because that is
invalid at this point. The design would need to change for that.
Also the c_array() function is a good idea.
It is documented in your header but not implemented. I think it is a
bad idea myself and I explain that in the code. But in an effort to
provide the advertised design of dynamic_2d_array I added the
functionality against my better judgement....I did that a lot here.
The iterators would have been a good idea, if you would have added the
begin and end function.
Actually, no they are a bad idea...or at least much more involved that
you think. I'm removing the concept.
The necessity of begin()/end() is debatable but it would be consistant
with the STL in that ONE regard.
NEW CODE==================
// This class reimplements the dynamic_2d_array template created
// by David Maisonave and can be viewed at:
//
http://code.axter.com/dynamic_2d_array.h
//
// This version fixes some of the bugs and poor coding practices in the
above
// implementation. I do not recommend doing anything like this, but if
you
// must, and want an example, I can't leave you at the mercy of a coder
that
// comes up with the above and calls himself an expert.
//
// Note: I am also not an expert. There are very likely places where
this could
// be improved. My heart is not into it as I disagree with the very
idea.
#include <cstdlib>
#include <cassert>
#include <algorithm>
// Use typename or class? It doesn't really matter. One could
succesfully
// argue that the primary purpose of this class isn't to contain
classes though.
template< typename T >
class dynamic_2d_array
{
public:
dynamic_2d_array(size_t rowCount, size_t colCount)
: rowCount_(rowCount), colCount_(colCount),
storage_(new T[rowCount * colCount]())
{
/*
* Changes: more descriptive variable names and a lack for a check
* of size for 0. Allocate any time. The reason for the
* later is simple: the check doesn't help, ignores the user's
* intention, and causes undefined behavior if the class is ever
* used.
*
* The correct way to do what the original code does, making sure
this
* class isn't used with 0 bounds is:
*/
assert(rowCount > 0 && colCount > 0);
/*
* You could throw an exception instead. All depends on if this
is an
* incorrect use of the class or a runtime error. I'm choosing
incorrect
* use of the class. Seems logical and is easier to implement.
*/
}
dynamic_2d_array(const dynamic_2d_array & other)
: rowCount_(other.rowCount_), colCount_(other.colCount_),
storage_(new T[other.rowCount_ * other.colCount_]())
{
// In this case there is no need for the check at all as the
creation
// of a dynamic_2d_array with 0 bounds has been disallowed.
// Assert anyway to advertise the precondition...
assert(other.rowCount_ > 0 && other.colCount_ > 0 && other.storage_
!= 0);
std::copy(other.storage_,
other.storage_ + (rowCount_ * colCount_),
storage_);
}
~dynamic_2d_array() { delete [] storage_; }
// These definitions hide the fact that you are actually returning
// a pointer to insides. Clients are discouraged from depending on
this fact
// as it is not advertized by the interface. You could change this
at any
// time and not affect well behaved clients. Without these
definitions any
// changes would propegate to all callers.
typedef T* row_reference;
typedef const T* const_row_reference;
// Col reference is much more complex and not in the original
interface.
// Personally I think the interface is not complete without this
definition
// but am not willing to spend the time implementing it. For one,
operator[]
// would then be totally inappropriate and implementing that operator
is the
// purpose of the original class.
// Why not "iterators"? Imagine using ++ on one and what you would
get.
// These are not row iterators and any other type would be bad
design.
// I'm too lazy to implement row iterators correctly - just as
complex as
// a col reference would be. That is an exercize left to the reader.
row_reference operator [] (size_t row)
{
return storage_ + (row * colCount_);
}
const_row_reference operator [] (size_t row) const
{
return storage_ + (row * colCount_);
}
// That is the extent of the original dynamic_2d_array. Those are
all the
// messages that object can respond to. Not very dynamic. Here are
some
// more messages that will let this class at least be useful in that
it will
// do MORE, not LESS, than a vector or primitive array.
// bounds checking implementations of the above:
row_reference row(size_t row)
{
if (!(row < rowCount))
{
throw std:
ut_of_range("Row index out of range");
}
return storage_ + (row * colCount_);
}
const_row_reference row(size_t row) const
{
if (!(row > rowCount))
{
throw std:
ut_of_range("Row index out of range");
}
return storage_ + (row * colCount_);
}
// No way to protect the row reference from invalid col bounds
because of the
// implementation. Changing row_reference to a smarter object could
fix that
// and allow a true row_iterator.
// provide sizing knowledge, needed by almost any client:
size_t rowCount() const { return rowCount_; }
size_t colCount() const { return colCount_; }
// provide resizing - make the class live up to its name:
void resize(size_t rowCount, size_t colCount)
{
assert(rowCount > 0 && colCount > 0);
T * temp_storage = new T[rowCount * colCount]();
size_t rows = std::min(rowCount, rowCount_);
size_t cols = std::min(colCount, colCount_);
for (size_t i = 0; i < rows; ++i)
{
std::copy(storage_ + (i * colCount_),
storage_ + (i * colCount_) + cols,
temp_storage + (i * colCount));
}
// operations that could cause an exception are finished.
// None of the following can. Strong guarantee...
std::swap(storage_, temp_storage);
rowCount_ = rowCount;
colCount_ = colCount;
delete [] temp_storage;
}
/*
* The lack of this operator is a major deficiency of the original
class.
* At one point it was hidden under the "protected" scope and I
mentioned that
* this was not the best as the class cannot be overriden anyway.
The
* original was better at that time but has since been "improved" to
not have
* or hide this operator. This means it is implemented by default
and will
* generate undefined behavior the first time it is used and the
objects
* destroyed. */
dynamic_2d_array & operator = (dynamic_2d_array other)
{
// Strong exception guarantee...
std::swap(storage_, other.storage_); // If this is new to you look
closely.
std::swap(rowCount_, other.rowCount_);
std::swap(colCount_, other.colCount_);
/* note that 'other' now owns my old storage and will delete it
when this
* function exits. */
return *this;
}
// To finish the *advertized* interface of the class as commented in
the
// original we must provide direct access to the data storage for C
functons
// that need it. This is reasonable but dangerous.
// Probably better ways to do this:
T * c_array() { return storage_; } // no reason to pretend we are not
doing
// what we are doing...
// Comments should reflect the inherint danger of calling the above
function
// at the least. Use should be highly discouraged. Better yet,
don't
// implement it at all and/or do something better. Perhapse the more
costly
// but more correct way:
// std::auto_ptr<T> c_array() const;
/*
* other useful interface items that could be implemented:
* * explicit constructor from c array.
* * functional interface to get T object as f() or operator ().
*
* Do not be tempted to implement a non-explicit constructor from a c
array
* or to implement an implicit conversion to T* or T**.
*/
private:
size_t rowCount_;
size_t colCount_;
T * storage_;
};
