Guru of the Week #4: Class Mechanics
Bradd W. Szonye
Herb Sutter <he...@cntc.com> wrote in article
<5fqhd5$5...@netlab.cs.rpi.edu>...
> GotW #4: Class Mechanics
>
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
First, I'd recommend against naming members with merely an underscore
prefix or suffix. This particular case isn't a reserved name, but it's
still difficult to distinguish _real from real. In general, underscore
suffixes are preferable to prefixes (because of the reserved identifier
rules) but both are inferior to a more-visually-distinguishable wart. (This
is probably the result of a corporate style standard; my own client has
such a standard, and our project has chosen to use the more easily
distinguished m_ and s_ prefixes.)
It's probably not unreasonable to provide an implicit conversion from real
doubles, but the decision influences other methods in this class.
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
And the first method is operator+. Any time there's an implicit conversion
allowed, non-assignment binary operators should be out-of-class and
implemented in terms of an assignment-operator. Otherwise, the RHS argument
will convert implicitly but the LHS argument will not. While implementation
in terms of += is not necessary, there is a simple idiom which gives you
both for "almost free" compared to the price of one--and neither operator
needs to be a friend.
Complex& Complex::operator+=(Complex const& other) {
_real += other._real;
_imaginary += other._imaginary;
return *this;
}
Complex operator+(Complex const& lhs, Complex const& rhs) {
Complex tmp = lhs;
return tmp += rhs;
}
Note also that all Complex arguments change from pass-by-value to
pass-by-reference; it's a borderline decision, but a good rule-of-thumb is:
if the member data is larger than an int (or double for
floating-point-based data), pass by const reference. I choose int and
double as the boundaries because those are the "default" type resulting
from integral or floating-point promotion.
(This operator+ is all mixed up: it implements assignment semantics and
returns no value. Operator+ should add the results into the temporary,
returning the temporary by value; operator+= should return a reference to
*this.)
While this class has appropriate shallow-copy semantics, Lakos would
recommend defining the copy-constructor explicitly anyways, on the grounds
that any class that actually *uses* its copy constructor may suffer from a
change in copy semantics in the future; you don't want all your clients to
recompile because of an implementation-only change.
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
Again, this method is completely wrong. However, where you can get away
with implementing operator+ in-class (even though your implicit conversions
won't balance correctly), inserters and extractors simply must be
out-of-class. The first argument must be an ostream& or istream&, which is
impossible for an in-class definition. Since there are no real/imaginary
accessors, the inserter must be a friend; its declaration should be
ostream& operator<<(ostream& os, Complex const& x) {
return os << "(" << x._real << "," << x._imaginary << ")";
}
for a minimal inserter. To really do the job correctly, you need to follow
Plauger's guidelines in "The (Draft) Standard C++ Library"; as is, this
improved inserter still won't correctly handle things like width
specifiers. Width of a complex is tricky; does the width pertain to the
overall output or each individual component? In any case, this inserter
dodges the issue entirely.
For those less familiar with streams: a minimal inserter/extractor takes a
non-const stream reference as a first argument and returns that argument
after the operation itself. The second argument should be the output/input
object as either a value parameter or a const-reference parameter
(depending on the size of the object).
> Complex operator++() {
> ++_real;
> return *this;
> }
Pre-increment is another function which needs to return a reference, not a
temporary. While there may be arguments over whether it's preferable to
return a const or non-const reference, that it be a reference is mandatory.
The more important problem, however, is the semantics of increment for a
complex number. (Is increment allowed even for doubles? Is a narrowing
conversion implied? Ick.) Increment is poorly defined for real numbers, let
alone complex numbers. This version increments the real component; an
equally-good definition would increment the magnitude or even phase angle.
This method is incorrect not only for its interface but for being in the
class in the first place.
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
Again, increment of any sort is probably inappropriate for complex numbers.
> private:
> double _real, _imaginary;
Again, preferable names would be m_real, thisReal, myReal, or even just
"real". My distaste for underscore warts is stylistic, but the underlying
reason is based in cognition/perception theory.
> };
After reviewing this code, I would question whether the responsible
programmer built a test harness for this code (or even compiled it, for
that matter). Things like the incorrect operator+ and stream insertion
operator would not survive even the simplest test harness.
I hope I've found all the problems in the code as written. There are so
many that it's easy to overlook any that might remain. The programmer
responsible for this code could definitely use some mentoring (if resources
are available). It reflects a level of experience consistent with an
early-intermediate programmer who fully understands the language syntax but
does not yet understand idioms beyond the most basic. While I can
understand overlooking things like not handling stream width() correctly--I
had written entire stream-derived classes and worked in C++ for six years
before learning this distinction--getting the prototype completely wrong is
a much more basic problem. Hopefully a decent teacher/mentor would have
time to set this (hypothetical?) programmer aright.
--
Bradd W. Szonye
bra...@concentric.net
http://www.concentric.net/~bradds
[ Send an empty e-mail to c++-...@netlab.cs.rpi.edu for info ]
[ about comp.lang.c++.moderated. First time posters: do this! ]
Jesse Jones
In article <5fqhd5$5...@netlab.cs.rpi.edu>, he...@cntc.com wrote:
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
>
>
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
I think it'd be better style to use a floating point literal when assigning
to a floating point variable. Also the semi-colon at the end is
superfluous.
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
This behaves more like += (without chaining) than +. It should be rewritten
like this:
Complex operator+(const Complex& rhs) const
{
Complex temp;
temp.mReal = mReal + rhs.mReal;
temp.mImaginary = mImaginary + rhs.mImaginary;
return temp;
}
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
os should be passed by reference and a reference to os should be returned.
I would also outline this function so that ostream could be forward
referenced in the header.
> Complex operator++() {
> ++_real;
> return *this;
> }
The convention I've seen (eg in STL) is for the prefix ++ operator to
return a reference. I would also question having increment operators in a
complex number class: it doesn't seem like a natural or intuitive
operation.
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
It'd be better style to write this using the prefix operator (++*this
instead of ++_real).
> private:
> double _real, _imaginary;
> };
Using identifiers with leading underscores is poor style; I'd use mReal and
mImaginary instead.
This class is also missing a lot of functionality. At the very least it
should have a default ctor and a set of accessors for _real and _imaginary.
-- Jesse
Marcelo Cantos
he...@cntc.com (Herb Sutter) writes:
> GotW #4: Class Mechanics
>
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
The default argument should be 0.0, not 0.
Prefix underscores are legal, but risky, since many compiler libraries
do not honour the reserved identifier naming rules.
real and imag would be better than _real and _imaginary (I'm *really*
reaching here). Perhaps even re and im. (Or *even* r and i!)
The semicolon after the {} is unnecessary.
Missing a default constructor.
A better implementation:
Complex() : re(0.0), im(0.0) { }
Complex(double re_, double im_ = 0.0) : re(re_), im(im_) { }
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
Legal, but incorrect semantics: + should add two numbers and return
the result as a new number. To do the above operation, we should use
operator+= and return *this.
`other' should be passed as a const reference.
_real += other._real is better than _real = _real + ... (likewise
_imaginary...).
The better way:
Complex& operator+=(const Complex& c) {
re += c.re;
im += c.im;
}
Complex operator+(const Complex& c) const { return Complex(*this) +=
c; }
also missing are all the other arithmetic operators.
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
This should be a global function that takes a reference to an ostream,
a const reference to Complex and returns an ostream.
Too simplistic: the user may wish to do this:
Complex c;
// ...
cout << setprecision(5) << setw(10) << c;
Since some flags only apply to the next output operation, this will
produce unexpected results. (Frankly, I can't be bothered working out
how this is done properly.)
The surrounding parentheses should be optional, and hence inserted by
the user.
Missing istream operator.
A better approach (which doesn't solve the manipulators problem):
[inside class definition]
friend ostream& operator<<(ostream& os, const Complex& c);
friend istream& operator>>(istream& is, Complex& c);
[outside class definition]
ostream& operator<<(ostream& os, const Complex& c) {
os << c.re << "," << c.im;
return os;
}
istream& operator>>(istream& is, Complex& c) {
is >> c.re >> c.im;
return is;
}
> Complex operator++() {
> ++_real;
> return *this;
> }
This operator is meaningless to a complex number. Leave it out.
>
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
Ditto.
>
> private:
> double _real, _imaginary;
Change names as indicated above, to make life easier on the author of
the class.
> };
Other missing items:
1. accessor members for real, imag, length, angle.
2. A way to construct a complex from polar coordinates.
3. Some handy constants (Complex::zero, Complex::one, Complex::i).
--
______________________________________________________________________
Marcelo Cantos, Research Assistant mar...@mds.rmit.edu.au
Multimedia Database Systems Group, RMIT__/_ _ Tel 61-3-9282-2497
723 Swanston St, Carlton VIC 3053 Aus/ralia ><_> Fax 61-3-9282-2490
/
Acknowledgements: errors - me; wisdom - God; funding - RMIT
Phil Weiss
Herb Sutter wrote:
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
>
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
>
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
[ other methods removed for brevity ]
>
> private:
> double _real, _imaginary;
> };
I agree with all the comments that Brad Szonye made regarding this
class. My comments are in addition to his. [Be nice to me, though; I'm
not a guru]
My suggestion is to properly encapsulate the "real" and "imaginary"
parts of the implementation into accessor functions. This class doesn't
lend itself well to change or to derivation.
If the implementor were to change his/her mind and store the complex in
a different manner (say with angle/magnitude) he/she would then have to
modify all functions which directly use the members.
Perhaps the class user wants a stream insertion operator with different
semantics. The user then derives a new class from this class. However,
as currently implemented, he has no access to Complex data.
Phil Weiss.
Pulkkinen Esa
he...@cntc.com (Herb Sutter) writes:
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
I'm first discussing the minor errors because otherwise I wouldn't
address them at all here - since the design would change completely :)
[1] First, I'd change the local variable's names to real_ and imaginary_,
because identifiers starting with an underbar are reserved for
C implementations.
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
I suppose it's intended that there is an implicit conversion from double
to complex, but I would put a comment for it so I wouldn't put explicit
to it someday :) Also I'm little suspicious of the 0 converted to
double, but I think that doesn't bother the compiler - just the human
reader....
>
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
[2] This is a gross abuse of the operator overloading mechanism. This
doesn't implement operator+, it does actually implement operator+=!
Also the additions inside the function are overly inefficient[3], even
though some compilers might be able to optimize it. And reference
parameter passing should be used [4]. Also this operation should return the
reference to *this to allow chaining[5].
So it should really be:
Complex& operator+=(const Complex& other) {
real_+=other.real_;
imaginary_+=other.imaginary_;
return *this;
}
>
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
This should be a friend function, otherwise the first argument isn't
an ostream[6]!
Copying ostream objects[7]? Hmm... I wonder if it's even legal. And anyway,
operator<< should return an ostream& [8]. Also it's more efficient (I guess)
to output characters and not strings [9]. So this becomes:
friend ostream& operator<<(ostream& os,const Complex& obj)
{
return os << '(' << obj.real_ << ',' << obj.imaginary_ << ')';
}
[10] There's one additional problem with this. Consider that this was
used as:
Complex x(10,20);
cout << setw(10) << x << endl;
This formats the number as "( 10,20)" (which takes far too much
space) and not as " (10,20)" which perhaps was intended.
To correct this problem you pad it yourself like this:
friend ostream& operator<<(ostream& os, const Complex& obj)
{
ostrstream temp; // hmm.. could use ostringstream if there was one in
// my compiler :)
temp.setf(os.flags());
temp.precision(os.precision());
if (os.flags() & ios::internal) temp.width((os.width()-2)/2);
temp << '(' << obj.real_ << ',';
if (os.flags() & ios::internal) temp.width((os.width()-3)/2);
temp << obj.imaginary_ << ')';
char *buf = temp.str();
os << buf;
free(buf);
return os;
}
Of course since this became a little lengthy, it shouldn't be contained
in the same file with all the declarations.
> Complex operator++() {
> ++_real;
> return *this;
> }
Again reference return value should be used when returning *this [11]!
>
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
>
> private:
> double _real, _imaginary;
> };
This class is also missing a copy-constructor (I would at least put a
comment that it's intentionally compiler-generated), assignment
(similarly here) and a default constructor. For the
default constructor, I'd perhaps make a default constructor initializing
both real and imaginary part to zero (otherwise the default constructor
doesn't initialize the members correctly)[12].
[I'm not counting the design problems as "errors", because it's hard to
count them individually]:
Overall, I think this class's design needs lots of work. First, it would
at least need access methods to the real and imaginary part of the
complex number, and similarly for the polar coordinates. Then the only
way you can change the value of the complex number is by adding to the
value - you would in practice need at least setting the value from the
coordinates (and likewise for polar coordinates), the missing addition,
substraction (including operator--(), operator-=() etc), multiplication
and division, negation operators. Then you could add more operations as
needed.
To implement most of this you could use the algebraic structure
categories from "Scientific and Engineering C++" by Barton&Nackman.
Second design issue is that perhaps this could be generalized so that
you could use different accuracy complex numbers - you won't probably
need the accuracy provided by doubles all the time - sometimes floats or
custom variable-precision floating point arithmetic is more
appropriate. Thus this could be parametrized with the parameter type.
Of course - some companies (still) disallow the use of templates, so
this might not be an issue.
--
Esa Pulkkinen | C++ programmers do it virtually
E-Mail: es...@cs.tut.fi | everywhere with class, resulting
WWW : http://www.cs.tut.fi/~esap/ | in multiple inheritance.
John David Duncan
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
I also don't like that semicolon.
>
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
this class.
> }
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
ostream << Class1 << Class2
this definition requires
Class1 << ostream .
insertion operator overrides should be declared
friend ostream & operator << (Complex &, ostream &)
and should return the ostream so that the operators can be evaluated
along a line, so we don't have separate statements.
> Complex operator++() {
> ++_real;
> return *this;
> }
better optimization:
Complex operator++()
{
return Complex(++real, imaginary)
}
>
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
Complex& operator++( int )
{
return Complex(_real++, _imaginary++)
}
>
> private:
> double _real, _imaginary;
> };
as a general stylistic note, to waste space here,
most people seem to prefer code (esp. contributed
code) to be written in this form:
class Name
{
private:
Type1 data1;
Type2 data2;
public:
Name(Type1 a = A_DEFAULT, Type1 b = B_DEFAULT)
: data1(a), data2(b)
{}
type1 getData1()
{
//code
}
void setData1(Type1 data)
{
//code
}
};
Ross Smith
Herb Sutter wrote:
>
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
>
(1) You shouldn't be writing a complex number class in the first place.
You should be using the standard one.
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
(2) Anything with a leading underscore is part of the reserved
namespace. (Well, technically that's not quite true, but the exceptions
are sufficiently difficult to remember that it's easiest just to never
use leading underscores. Anyway, these particular examples *are*
reserved.)
(3) The trailing semicolon shouldn't be there.
(4) This one is a stylistic issue on which there's serious disagreement,
but I'm with the school of thought that says you should always write an
explicit default constructor, copy constructor, destructor, and
assignment operator (if you really don't want some of them, make them
private). In this specific case the default semantics do the Right Thing
anyway (with the possible exception of the nonexistent default
constructor), but I'd still argue that it's wrong.
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
(5) This one is a disaster. For a start, it calls itself op+ but
implements op+= semantics.
(6) The argument should be passed by reference.
(7) Assuming it's really meant to be op+=, it should return a
self-reference by convention.
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
(8) Another disaster. The inserter *has* to be a friend, because the LHS
has to be an ostream.
(9) The ostream has to be passed by reference, because streams don't
copy. This one won't compile, unless you have a really weird
(nonstandard) streams package.
(10) It should return a reference to the ostream, for chaining.
> Complex operator++() {
> ++_real;
> return *this;
> }
(11) Another debatable style point: I think op++ and the like should
really be used only with integers and integer-like types. Even if you
disagree (and I can't actually remember whether op++ works on the
standard FP types), it's debatable what it should do when applied to a
complex.
(12) The return value should be a reference.
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
>
> private:
> double _real, _imaginary;
> };
(13) The class is hopelessly short of member functions. It looks as
though the implementor only put in the functionality [I hate that word
but I've never been able to come up with a better one] that was needed
for a specific application, making no allowance for re-use.
(14) Writing the whole class inline in the header file is almost always
a Bad Thing. It should be done only when the class is *really* small and
you can be *certain* it will never expand (certainly not true here!), or
for template classes when portability is more important than efficiency.
--
Ross Smith .................................... Wellington, New Zealand
Webmaster, EDS (New Zealand) Ltd ....... <mailto:ross....@nz.eds.com>
"I'm as interested as anybody else in all the things no decent
person would be interested in." -- Ashleigh Brilliant
Venkat Shrinivas
If this was a real code review, I would recommend we use the complex class
in STL instead ;-)
Marcelo Cantos
John David Duncan <jdds...@pitt.edu> writes:
> as a general stylistic note, to waste space here,
> most people seem to prefer code (esp. contributed
> code) to be written in this form:
>
> class Name
> {
> private:
> Type1 data1;
> Type2 data2;
> public:
> // ...
> };
Anyone (other than the class designer) who even wants to see the
private section, let alone see it first, doesn't understand
encapsulation.
--
______________________________________________________________________
Marcelo Cantos, Research Assistant mar...@mds.rmit.edu.au
Multimedia Database Systems Group, RMIT__/_ _ Tel 61-3-9282-2497
723 Swanston St, Carlton VIC 3053 Aus/ralia ><_> Fax 61-3-9282-2490
/
Acknowledgements: errors - me; wisdom - God; funding - RMIT
[ Send an empty e-mail to c++-...@netlab.cs.rpi.edu for info ]
David A. Cuthbert
Herb Sutter <he...@cntc.com> wrote:
>You are doing a code review. A programmer has written the following class,
>which shows some poor style and has some real errors. How many can you
>find, and how would you fix them?
Hmm... I think that people are going to have a field day with this one.
:-)
To summarize before my re-coding:
Template the class
Don't use underscore prefixes (use suffixes instead)
operator + should be +=
Take const references whenever possible
Define many more operators
Do NOT define non-intuitive operators (like Complex + real;
this destroys the view of Complex as a phasor, a typical
use of complex numbers); have the user use
Complex + Complex(real) instead.
Do NOT define operators (like ++) which give a notion for
ordering (complex numbers have no order)
Define certain operators (like <<) outside of the class
Define logical accessors and mutators (not just those that
act directly on the types -- e.g., re(), im(), arg(), mag())
I've debated removing << entirely. Different fields use different
representations (the default here is "(a,b)"). Physicists, for example,
would probably want "a + i b" notation; Mathematica users would insist on
"a + I b"; and we screwy electrical engineers insist on using j (i is used
for current), ala "a + j b" or sometimes "m ang(theta)" where m is the
magnitude, and theta is the angle/phase.
In short, a single definition of << isn't very flexible.
I've also debated making the default constructor the ONLY constructor;
allowing real and imaginary arguments biases the class toward Cartesian
coordinates rather than polar. In the end, I left it in because, if I
had to guess, Cartesian is used more often than polar.
> class Complex {
// Use a template instead -- allows for exact representation of
// integral types, long doubles instead of doubles, etc.; better
// code reuse
template <typename T>
class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
// No prefix underscores (danger of using reserved names; these
// are technically ok, but a bad idea)
// Default on the real, as well
// Take arguments by const reference in case the numeric type is
// a non-PODS itself
Complex(T const& real = 0, T const& imaginary = 0)
: real_(real), imaginary_(imaginary) { };
> void operator+ ( Complex other ) {
// I think that
void operator += (Complex<T> const& other) {
// is wanted -- take rval by const reference
> _real = _real + other._real;
// typename T may not have + defined; more efficient (less
// chance of compiler generating a temporary) if += is used
real_ += other.re(); // use accessor instead
> _imaginary = _imaginary + other._imaginary;
// Same story
imaginary_ += other.im();
> }
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
// Just plain wrong; use
friend ostream& operator << (ostream&, Complex<T> const&);
// and define << elsewhere
> Complex operator++() {
> ++_real;
> return *this;
> }
>
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
// Delete both of these -- complex numbers do not have an ordering,
// therefore increment (and decrement) is not an intuitive operation.
// Now, some accessors and mutators that are needed:
T const& re() const {
return real_;
}
T const& im() const {
return imaginary_;
}
Complex<T>& re(T const& real) {
real_ = real;
return *this;
}
Complex<T>& im(T const& imaginary) {
imaginary_ = imaginary;
return *this;
}
// More "accessors" that are natural operations on a complex
// number
long double arg() const {
return arctan2l(imaginary_, real_);
}
long double mag() const {
return sqrtl(imaginary_ * imaginary_ + real_ * real_);
}
Complex<T>& arg(long double angle) {
long double currentMagnitude = mag();
real_ = currentMagnitude * cosl(angle);
imaginary_ = currentMagnitude * sinl(angle);
return *this;
}
Complex<T>& mag(long double magnitude) {
long double currentAngle = arg();
real_ = magnitude * cosl(currentAngle);
imaginary_ = magnitude * sinl(currentAngle);
return *this;
}
// More operators are needed -- I'll provide only prototypes
// here
void operator -= (Complex<T> const&);
void operator *= (Complex<T> const&);
template <typename numericRealT>
void operator *= (numericRealT const&);
void operator /= (Complex<T> const&);
template <typename numericRealT>
void operator /= (numericRealT const&);
> private:
> double _real, _imaginary;
// Go to single line/declaration:
T real_;
T imaginary_;
> };
// Define template<typename T> ostream& operator << (ostream&,
// Complex<T> const&) here
// More operators (Complex<T>& operator +-*/ (Complex<T>&, Complex<T> const&)
// Override standard math functions (pow, sqrt, log, sin, cos, tan, exp, etc.)
// Note that log(z) == log(abs(z)) + j arg(z); (abs(z) == z.mag())
// Likewise, sin(z) == (exp(j z) - exp(-j z)) / 2j, etc.
There's the issue of whether to make the methods virtual. I probably
would not, since the overhead of a vtbl can be excessive if Complex is
used in an iterative algorithm (Newton's method, for example). In that
case, put big comments stating that this is a leaf class.
Overall, more comments are needed. Also, move the implicit inline
functions into a .inl file (or .cpp file).
Steve Willer
Pulkkinen Esa <es...@cs.tut.fi> wrote:
>This class is also missing a copy-constructor (I would at least put a
>comment that it's intentionally compiler-generated), assignment
>(similarly here) and a default constructor.
I strongly disagree with the creation of a copy constructor and assignment
operator. It is infinitely preferable to make sure that your class's members
are copyable (i.e. no pointer or auto_ptr members) and leave the creation of
these special functions to the compiler. The moment you start building your own
copy constructor and assignment operator, you leave yourself open to the big
demon: HUMAN ERROR. Perhaps you forgot a member. Perhaps you add a member
later on and forget to change the copy constructor and/or assignment operator.
Far too much risk, and for no gain.
> For the
>default constructor, I'd perhaps make a default constructor initializing
>both real and imaginary part to zero (otherwise the default constructor
>doesn't initialize the members correctly)[12].
I think this makes sense.
>Overall, I think this class's design needs lots of work. First, it would
>at least need access methods to the real and imaginary part of the
>complex number, and similarly for the polar coordinates. Then the only
>way you can change the value of the complex number is by adding to the
>value - you would in practice need at least setting the value from the
>coordinates (and likewise for polar coordinates), the missing addition,
>substraction (including operator--(), operator-=() etc), multiplication
>and division, negation operators. Then you could add more operations as
>needed.
True. I overlooked all that stuff.
Steve Willer
he...@cntc.com (Herb Sutter) wrote:
> class Complex {
> public:
> Complex( double real, double imaginary = 0 )
> : _real(real), _imaginary(imaginary) {};
You shouldn't use underscores at the beginning of symbols. Someday a compiler
may bite you for that one (something about underscores at the beginning of
symbols running into the namespace allowed for library implementors).
I don't like "imaginary" having a default value. This creates a single-argument
constructor, which can be used for implicit conversion. I don't see any strong
justification for having the default value here, so it should be removed.
> void operator+ ( Complex other ) {
> _real = _real + other._real;
> _imaginary = _imaginary + other._imaginary;
> }
The argument should be of type "const Complex&". The code inside is pretty poor
because it uses + instead of +=, which is somewhat misleading.
In this particular case, it misleads the reader into thinking that operator+ is
what's being implemented here, while the code fits an operator+ more.
This operator+ should be returning a Complex (by value) and should be const. A
more correct implementation would be:
Complex operator+ (const Complex& other) const {
return Complex(_real+other._real, _imaginary+other._imaginary);
}
> void operator<<( ostream os ) {
> os << "(" << _real << "," << _imaginary << ")";
> }
Passing a stream by value? Yikes! That'll almost certainly crash on gcc.
The fact that operator<< is a member function means this operator will only get
used if the Complex is on the left and the stream is on the right. I'm sure
this isn't what's expected. The operator should be in global scope and should
take an ostream& on the left and a "const Complex &" on the right.
This operator<< is returning a void when it should be returning an ostream&. It
should return "os". The operator<< should also be declared a friend within the
class definition.
> Complex operator++() {
> ++_real;
> return *this;
> }
This should return a Complex*.
Design quibble: I don't like having an operator++ at all, because it's not
clear to me what's being incremented. If it's unclear whether r or i will be
incremented, the client is bound to get confused and make mistakes. I say throw
away this operator.
> Complex operator++( int ) {
> Complex temp = *this;
> ++_real;
> return temp;
> }
There really isn't any reason to ever implement a postfix operator++. I just
don't do it and thereby force clients to use the preferred prefix form. On top
of this reason, this function should be thrown away because of the reasons
outlined for the prefix ++.
Steve Willer
John David Duncan <jdds...@pitt.edu> wrote:
>> void operator+ ( Complex other ) {
>style: Complex parameter should be 'const Complex &'
>> _real = _real + other._real;
>> _imaginary = _imaginary + other._imaginary;
>error: other._real and other._imaginary cannot be accessed by
> this class.
Can so. Access specifiers are per class, not per object.
Marcelo Cantos
Phil Weiss <we...@oak.pacsim.com> writes:
>
> Herb Sutter wrote:
>
> > You are doing a code review. A programmer has written the following class,
> > which shows some poor style and has some real errors. How many can you
> > find, and how would you fix them?
> >
> > class Complex {
> > public:
> > Complex( double real, double imaginary = 0 )
> > : _real(real), _imaginary(imaginary) {};
> >
> > void operator+ ( Complex other ) {
> > _real = _real + other._real;
> > _imaginary = _imaginary + other._imaginary;
> > }
>
> [ other methods removed for brevity ]
>
> >
> > private:
> > double _real, _imaginary;
> > };
>
> I agree with all the comments that Brad Szonye made regarding this
> class. My comments are in addition to his. [Be nice to me, though; I'm
> not a guru]
>
> My suggestion is to properly encapsulate the "real" and "imaginary"
> parts of the implementation into accessor functions. This class doesn't
> lend itself well to change or to derivation.
This topic has been beaten to death before. Accessors for private
members are a complete waste of effort.
> If the implementor were to change his/her mind and store the complex in
> a different manner (say with angle/magnitude) he/she would then have to
> modify all functions which directly use the members.
>
Most of the functions would have to be modified anyway!
This was actually one of the problems that the exercise is intended to
illuminate. However, this doesn't mean that the implementation should
use these accessors. The reason is that different implementations
encourage different optimisations, for instance:
// Cartesian implementation
complex operator*(const complex& c, double r) {
c.re *= r;
c.im *= r;
}
// Polar implementation
complex operator*(const complex& c, double r) {
c.rad *= r;
}
if accessors were used, the implementation might be:
complex operator*(const complex& c, double r) {
c.setReal(c.real() * r);
c.setImag(c.imag() * r);
}
Now, this is fine if the implementation is cartesian, but what if it
were polar?...
double real() { return rad * cos(ang); }
double setReal(double re) {
double im = imag();
rad = sqrt(re * re + im * im);
ang = atan2(im, re);
}
// and likewise for imag() and setImag()...
A generic operator* would be absurdly expensive compared to the custom
built implementation. It would involve 10 multiplications, two
additions, two square roots and two atan2's! All floating point!!
You don't escape by implementing operator* with the polar accessors
either:
complex operator*(const complex& c, double r) {
setRad(rad() * r);
}
Since the cartesion implementation of rad() and setRad() would look
something like this:
double rad() { sqrt(re * re + im * im); }
double setRad(double rad) {
double ang = atan2(im, re);
re = rad * cos(ang);
im = rad * sin(ang);
}
This would mean that operator* incurs 5 multiplications, one addition,
one square root, one atan2, one cos and one sin. I can't tell which
is worse!
In summary, even if you *can* make member functions implementation
independent it is a REALLY bad idea.
> Perhaps the class user wants a stream insertion operator with different
> semantics. The user then derives a new class from this class. However,
> as currently implemented, he has no access to Complex data.
This seems to me to be a bad idea too, although I have no specific
reasons for not liking it. It just seems very odd to derive a new
class for no other reason than to modify printing behaviour. What
happens if you are passed a complex and what to change its printing
behaviour? Do you assign it to a derived class instance?
My personal opinion (and nothing more) is that complex data should be
immutable (ie: no set...() functions, only get...() function). In
order to change one part, you have to assign a new instance to the
current one. For instance, to add one to the real component, just use
c += 1.0. To set the imaginary component, use
c = Complex(c.real(), 7.0).
The latter operation would be rarely used anyway, since complex
numbers are generally used as self-contained entities, like real
numbers.
--
______________________________________________________________________
Marcelo Cantos, Research Assistant mar...@mds.rmit.edu.au
Multimedia Database Systems Group, RMIT__/_ _ Tel 61-3-9282-2497
723 Swanston St, Carlton VIC 3053 Aus/ralia ><_> Fax 61-3-9282-2490
/
Acknowledgements: errors - me; wisdom - God; funding - RMIT
[ Send an empty e-mail to c++-...@netlab.cs.rpi.edu for info ]
Achim Gratz
he...@cntc.com (Herb Sutter) writes:
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
If this were a code review I'd throw out the code on first sight and
put the programmer on probation for a) reinventing the wheel (even
really poorly) and b) not having understood what complex numbers are
about and still implementing a class Complex without approval.
Achim Gratz.
--+<[ It's the small pleasures that make life so miserable. ]>+--
WWW: http://www.inf.tu-dresden.de/~ag7/{english/}
E-Mail: gr...@ite.inf.tu-dresden.de
Phone: +49 351 463 - 8325
James Kanze
John David Duncan <jdds...@pitt.edu> writes:
|> > Complex( double real, double imaginary = 0 )
|> > : _real(real), _imaginary(imaginary) {};
|> style: double parameters should be 'const double &'
Why? All of the guidelines I've seen recommend pass by value for the
basic types, pointers and enums, pass by const reference for class and
unions, and never passing C style arrays.
|> I also don't like that semicolon.
How do you say "au point ou on en est" in English? I probably wouldn't
use it, but the code's unreadable, with or without. So who cares.
|> > void operator+ ( Complex other ) {
|> style: Complex parameter should be 'const Complex &'
|> > _real = _real + other._real;
|> > _imaginary = _imaginary + other._imaginary;
|> error: other._real and other._imaginary cannot be accessed by
|> this class.
Since when?
|> > }
|> > void operator<<( ostream os ) {
|> > os << "(" << _real << "," << _imaginary << ")";
|> > }
|> error: this will not work because of syntax of insertion operator:
|> ostream << Class1 << Class2
|> this definition requires
|> Class1 << ostream .
|> insertion operator overrides should be declared
|> friend ostream & operator << (Complex &, ostream &)
^^^^^^^ ^^^^^^^
A typo, to be sure, but the call syntax of your operator is the same as
that of a member function.
|> and should return the ostream so that the operators can be evaluated
|> along a line, so we don't have separate statements.
|> > Complex operator++() {
|> > ++_real;
|> > return *this;
|> > }
|> style: we should use the temporary object to allow for
|> better optimization:
|> Complex operator++()
|> {
|> return Complex(++real, imaginary)
|> }
What? In this case, the original will probably have less objects than
what you propose.
Returning a reference might not be a bad idea, however, and would
eliminate a temporary.
|> >
|> > Complex operator++( int ) {
|> > Complex temp = *this;
|> > ++_real;
|> > return temp;
|> > }
|> style: for optimization purposes this should be
|> Complex& operator++( int )
|> {
|> return Complex(_real++, _imaginary++)
|> }
For optimization purposes, there is no difference. (According to the
standard, at least. Real implementations may vary.)
|> > private:
|> > double _real, _imaginary;
|> > };
|>
|> as a general stylistic note, to waste space here,
|> most people seem to prefer code (esp. contributed
|> code) to be written in this form:
|>
|> class Name
|> {
|> private:
|> Type1 data1;
|> Type2 data2;
Almost every coding guideline I've seen requires public before private.
So where are your "most people seem to prefer"?
|> public:
|> Name(Type1 a = A_DEFAULT, Type1 b = B_DEFAULT)
|> : data1(a), data2(b)
|> {}
|> type1 getData1()
|> {
|> //code
|> }
|>
|> void setData1(Type1 data)
|> {
|> //code
|> }
|> };
--
James Kanze home: ka...@gabi-soft.fr +33 (0)1 39 55 85 62
office: ka...@vx.cit.alcatel.fr +33 (0)1 69 63 14 54
GABI Software, Sarl., 22 rue Jacques-Lemercier, F-78000 Versailles France
-- Conseils en informatique industrielle --
James Kanze
he...@cntc.com (Herb Sutter) writes:
|> You are doing a code review. A programmer has written the following class,
|> which shows some poor style and has some real errors. How many can you
|> find, and how would you fix them?
|>
To start with, there isn't the slightest indication of what this class
is supposed to do. What is its role? Its responsibilities? What is it
modelling?
>From the name, and the arguments, I'd guess it is a model of the
mathematical concept of complex numbers. But the functions don't seem
to concur: I see no way of accessing the attributes real and imaginary,
for example, nor any of the numerous mathematical operators that one
would expect. On the other hand, I wouldn't expect incrementation in
this case.
>From this, I am forced to conclude that it is not modelling the well
known mathematical concept. Which means that it is going to be very
hard to review, since I haven't the slightest idea what it is supposed
to do. I might start by saying that the name is misleading, since it
gives rize to expectations which are not kept.
|> class Complex {
|> public:
|> Complex( double real, double imaginary = 0 )
|> : _real(real), _imaginary(imaginary) {};
To start with: I don't like mixing implementation and interface. For
something this simple (and mathematical, too, where performance tends to
be important), I'd probably break my general rule, and use inline's even
before seeing profiling data, but I would put them outside of the class
definition, for readability.
I generally like to offer a default constructors where ever reasonable.
While it's hard to tell without knowing exactly what the class should be
doing, I rather suspect that it would be reasonable here.
I don't see any copy constructor, nor assignment operator. While I'm
not against using the compiler generated defaults, the comments should
make it clear that this is being done intentionally, and that I haven't
simply forgotten.
|> void operator+ ( Complex other ) {
|> _real = _real + other._real;
|> _imaginary = _imaginary + other._imaginary;
|> }
Again: not knowing what is being modelled makes it difficult to say what
the semantics of operator+ should be. Still, having them modify the
right operand, and not allowing them to chain ("a+b+c") looks more than
a little suspicious. As does having an operator+ without an operator+=.
Of course, if the desired semantics didn't modify the right operand, you
would probably not want the function to be a member.
|> void operator<<( ostream os ) {
|> os << "(" << _real << "," << _imaginary << ")";
|> }
This has to be wrong. I may not know the semantics of what is being
modelled, but I do know the semantics of ostream:
1. You simply cannot pass ostream by value, it doesn't support copying.
2. The standard idiom has the stream on the right, and the object to be
output on the left, and not the inverse.
3. The standard idiom returns a reference to the stream, for chaining.
And that's just the interface! The implementation ignores both the
formatting parameters (width, in particular), and the locale. (Note
that in most locale's, you cannot use ',' as a separator, since it is
generated as part of the double).
|> Complex operator++() {
|> ++_real;
|> return *this;
|> }
|>
|> Complex operator++( int ) {
|> Complex temp = *this;
|> ++_real;
|> return temp;
|> }
I've skipped the above two, since I have no idea what they are supposed
to do. On the whole, however, I doubt that whatever it is should be
represented by ++.
|> private:
|> double _real, _imaginary;
|> };
Using a leading underscore in names is inviting portability problems
later, regardless of what the standard says.
Globally, the formatting is too dense (perhaps due to wanting to make it
smaller for posting), with implementations in the middle of the
interface specification, several variables in the same declaration,
several initializers on the same line, etc., etc. I like to keep the
opening { on a separate line as well, but that is more a question of
personal taste, except for those at the outer level (which are
recognized by a number of editors).
Also, it's impossible to really review code (especially interfaces)
without some explainitory comments or documentation. Normally, I would
expect as a minimum a comment above the class, describing what is being
modelled, or the class' role and responsibilities in the application.
Similarly, a description of each function, including pre- and
post-conditions, would seem necessary; it's very hard to determine
whether a function meets its post-conditions if you don't know what they
are.
In all of the code reviews I've been to, the above code would have been
simply classes as "not ready for review" in the first five minutes.
After all, the people doing the reviewing have other work, as well.
James Kanze
Ross Smith <ross....@nz.eds.com> writes:
|> > public:
|> > Complex( double real, double imaginary = 0 )
|> > : _real(real), _imaginary(imaginary) {};
|>
|> (2) Anything with a leading underscore is part of the reserved
|> namespace. (Well, technically that's not quite true, but the exceptions
|> are sufficiently difficult to remember that it's easiest just to never
|> use leading underscores. Anyway, these particular examples *are*
|> reserved.)
Technically, they aren't in this context. But as you say, the rules are
complex enough to remember that a significant number of compilers don't
get them right:-).
[...]
|> (8) Another disaster. The inserter *has* to be a friend, because the LHS
|> has to be an ostream.
Just a nit, but insertion and extraction functions should not generally
be friends. If they are, they allow the user to get at the internal
representation. (But I think the point that you were making is that
they shouldn't be members, which is certainly true.)
James Kanze
da...@henry.ece.cmu.edu (David A. Cuthbert) writes:
[Concerning complex...]
|> I've debated removing << entirely. Different fields use different
|> representations (the default here is "(a,b)"). Physicists, for example,
|> would probably want "a + i b" notation; Mathematica users would insist on
|> "a + I b"; and we screwy electrical engineers insist on using j (i is used
|> for current), ala "a + j b" or sometimes "m ang(theta)" where m is the
|> magnitude, and theta is the angle/phase.
That's what ios::xalloc is all about. You probably want to define two
new sets of flags: mathematic, physic, electric and cartesian, polar,
kept on a word allocated with ios::xalloc. Of course, you'll need
corresponding manipulators. And documentation.
For the rest: I liked your comments a lot.
--
James Kanze home: ka...@gabi-soft.fr +33 (0)1 39 55 85 62
office: ka...@vx.cit.alcatel.fr +33 (0)1 69 63 14 54
GABI Software, Sarl., 22 rue Jacques-Lemercier, F-78000 Versailles France
-- Conseils en informatique industrielle --
[ Send an empty e-mail to c++-...@netlab.cs.rpi.edu for info ]
Jim Hyslop
Steve Willer <wil...@interlog.com> wrote in article
<5g37nt$g...@netlab.cs.rpi.edu>...
> Pulkkinen Esa <es...@cs.tut.fi> wrote:
>
> >This class is also missing a copy-constructor (I would at least put a
> >comment that it's intentionally compiler-generated), assignment
> >(similarly here) and a default constructor.
>
> I strongly disagree with the creation of a copy constructor and
assignment
> operator. It is infinitely preferable to make sure that your class's
members
> are copyable (i.e. no pointer or auto_ptr members) and leave the creation
of
> these special functions to the compiler. The moment you start building
your own
> copy constructor and assignment operator, you leave yourself open to the
big
> demon: HUMAN ERROR. Perhaps you forgot a member. Perhaps you add a
member
> later on and forget to change the copy constructor and/or assignment
operator.
>
> Far too much risk, and for no gain.
constructors in general. Shallow copying is fine for this class, but
the
moment you start adding pointers or references to a class, chances are
very
good that you will be **required** to add a deep copy constructor. If
you
provide a copy constructor (and assignment operator, for that matter)
from
the beginning, there's less chance (IMO) that you'll forget to add it
later
when you decide your class needs pointers or references.
--
Jim Hyslop
jim.h...@leitch.com
Ethical question: Is it OK to yell "Movie" in a crowded firehouse?
Katy Mulvey
In article <5g37ng$g...@netlab.cs.rpi.edu>, wil...@interlog.com (Steve Willer) wrote:
>he...@cntc.com (Herb Sutter) wrote:
>
>> class Complex {
>> Complex( double real, double imaginary = 0 )
>> : _real(real), _imaginary(imaginary) {};
>
>may bite you for that one (something about underscores at the beginning of
>symbols running into the namespace allowed for library implementors).
Actually, the draft standard only reserves a double underscore and a single
underscore followed by a capital letter for libraries. See [lex.name], section
2.10.2.
It's still ugly, though. And it's probably a bad habit to get into, since it's
awfully easy to accidentally name a class member incorrectly. In a real
code review, I could refer to the company's style guide, which would
indicate good and bad practices for naming member variables.
I agree with whoever it was who said that the best fix for this
*particular* class is:
#include <complex>
Katy (k...@ormec.com)
John E. Potter
David A. Cuthbert (da...@henry.ece.cmu.edu) wrote:
: // Use a template instead -- allows for exact representation of
: // integral types, long doubles instead of doubles, etc.; better
: // code reuse
: template <typename T>
: class Complex {
[ very nice ]
: // More "accessors" that are natural operations on a complex
: // number
: long double arg() const {
: return arctan2l(imaginary_, real_);
: }
: long double mag() const {
: return sqrtl(imaginary_ * imaginary_ + real_ * real_);
: }
I think we have a problem:
Complex< Zmod<5> > oops(2, 3);
Zmod<5> r(oops.mag());
Watch out for algebraists:-) I want a T not a long double.
John
John E. Potter
James Kanze (james-alb...@vx.cit.alcatel.fr) wrote:
: Ross Smith <ross....@nz.eds.com> writes:
: [...]
: |> (8) Another disaster. The inserter *has* to be a friend, because the LHS
: |> has to be an ostream.
: Just a nit, but insertion and extraction functions should not generally
: be friends.
Agree, minimise friends.
: If they are, they allow the user to get at the internal representation.
I don't see this. The user uses the non-member function which is part of
the class interface. How does that give the user access to internals?
John E. Potter
Pulkkinen Esa (es...@cs.tut.fi) wrote:
: Second design issue is that perhaps this could be generalized so that
: you could use different accuracy complex numbers - you won't probably
: need the accuracy provided by doubles all the time - sometimes floats or
: custom variable-precision floating point arithmetic is more
: appropriate. Thus this could be parametrized with the parameter type.
: Of course - some companies (still) disallow the use of templates, so
: this might not be an issue.
I guess this would also require at a minimum a conversion constructor
template<class U>
Complex (Complex<U> const& src) :
re(src.real()),
im(src.imaginary()) {
}
I hope T(U) is valid for all T and U.
John
John E. Potter
Herb Sutter (he...@cntc.com) wrote:
: GotW #4: Class Mechanics
: You are doing a code review. A programmer has written the following class,
: which shows some poor style and has some real errors. How many can you
: find, and how would you fix them?
Assumptions: The title "Class Mechanics" was intended to limit the
problem to implementation of the indicated semantics. The class is
of course incomplete and everything is inlined for exposition only.
The class name Complex means what it says and cartesian coordinates
where dictated. Any name could have been chosen, the objective is
the implementation of the members only. Now reduce the difficulty
level, and hand the programmer one of Scott Meyers' books.
: class Complex {
: public:
: Complex( double real, double imaginary = 0 )
: : _real(real), _imaginary(imaginary) {};
We have two constructors, one of which is conversion from real. Since
reals are a subset of complex, this seems reasonable. We are missing
a default constructor which makes containers and input difficult. May
as well add a default 0 for the real component and get three for the
price of one.
The other three compiler generated functions work fine and should be
left out. This is an ADT not intended for derivation and there will
be no changes which would include raw pointer data members.
: void operator+ ( Complex other ) {
: _real = _real + other._real;
: _imaginary = _imaginary + other._imaginary;
: }
The semantics is operator+=. Being pragmatic, operator+= is the member
to implement in the class (almost) always. Should also have operator+
as a non-member, non-friend for consistent lazyness. The size of
Complex is large enough to use const& parameter.
: void operator<<( ostream os ) {
: os << "(" << _real << "," << _imaginary << ")";
: }
Must have a buggy compiler that let this ostream pass by value get
through. And this is just not the way "everyone" does things. Let's
get aboard and not be surprisingly creative with the semantics of
stream output. I'm not sure that this should even be provided, but
let's say that the requirements included it. Move it out to a global
function for standard implementation and provide projection functions
to avoid needlessly making it a friend. The projections are useful
for other things also. They will be value semantics and I will not
add setters.
: Complex operator++() {
: ++_real;
: return *this;
: }
No religious wars please, but operator++() returns a reference.
: Complex operator++( int ) {
: Complex temp = *this;
: ++_real;
: return temp;
: }
This one is reasonable; however, I will move it out as a non-member,
non-friend for consistent lazyness. Implemented in terms of prefix.
: private:
: double _real, _imaginary;
I dislike warts in general and leading underscore is notoriously risky.
: };
Looking for comments,
John
class Complex {
public:
Complex ( double real = 0, double imaginary = 0 ) :
re(real), im(imaginary) {
}
double real () const {
return re;
}
double imaginary () const {
return im;
}
Complex& operator+= ( Complex const& other ) {
re += other.re;
im += other.im;
return *this;
}
Complex& operator++ () {
++ re;
return *this;
}
private:
double re, im;
};
ostream& operator<< ( ostream& os, Complex const& c ) {
// You asked for this format, you got it.
return os << "(" << c.real() << "," << c.imaginary() << ")";
}
// #include consistent.lazyness.h
template <class T>
T operator+ ( T const& lhs, T const& rhs ) {
return static_cast<T>(lhs) += rhs;
// One of my dumb compilers sees T(lhs) as const_cast<T&>(lhs)
}
template <class T>
T operator++ (T& it, int ) {
T temp(it);
++ it;
return temp;
James Kanze
jpo...@falcon.lhup.edu (John E. Potter) writes:
|> James Kanze (james-alb...@vx.cit.alcatel.fr) wrote:
|> : Ross Smith <ross....@nz.eds.com> writes:
|>
|> : [...]
|> : |> (8) Another disaster. The inserter *has* to be a friend,
because the LHS
|> : |> has to be an ostream.
|>
|> : Just a nit, but insertion and extraction functions should not
generally
|> : be friends.
|>
|> Agree, minimise friends.
|>
|> : If they are, they allow the user to get at the internal
representation.
|>
|> I don't see this. The user uses the non-member function which is
part of
|> the class interface. How does that give the user access to
internals?
Extract with ostrstream, modify the text, and insert with istrstream.
OK, it's pretty sneaky, really Machiavelli. My point, however, is that
what you output is part of the *external* interface; if something isn't
accessible through the public interface, it probably should not be part
of the data output to a stream.
--
James Kanze home: ka...@gabi-soft.fr +33 (0)1 39 55 85
62
office: ka...@vx.cit.alcatel.fr +33 (0)1 69 63 14
54
GABI Software, Sarl., 22 rue Jacques-Lemercier, F-78000 Versailles
France
-- Conseils en informatique industrielle --
[ Send an empty e-mail to c++-...@netlab.cs.rpi.edu for info ]
David A. Cuthbert
>: // number
>: long double arg() const {
>: return arctan2l(imaginary_, real_);
>: }
>
>: long double mag() const {
>: return sqrtl(imaginary_ * imaginary_ + real_ * real_);
>
>I think we have a problem:
>
> Complex< Zmod<5> > oops(2, 3);
> Zmod<5> r(oops.mag());
>
>Watch out for algebraists:-) I want a T not a long double.
Except I couldn't find a way to do this without requiring that T
support initialization from a double or long double (or that you
implement sqrt or arctan2). Assuming that T is an integral type, for
example, I would probably still want a fairly good result from arg()
and mag(), which can't happen if T is int, long, etc.
By doing this, I'm making the guarantee that Complex<T> has at least
the level of T precision in Cartesian coordinates; polar
representations are provided for convenience.
Actually, one solution would be to implement a templated member:
template<class Result> Result mag() { ...
but I tend to avoid them, since I'm still using a compiler that
doesn't support them. (A new compiler is on the way :-), but my
computer at home isn't powerful enough to run it :-( ).
--
David A. Cuthbert
Graduate Student, Electrical and Computer Engineering
Data Storage Systems Center, Carnegie Mellon University
John E. Potter
James Kanze <james-alb...@vx.cit.alcatel.fr> wrote:
: jpo...@falcon.lhup.edu (John E. Potter) writes:
: |> James Kanze (james-alb...@vx.cit.alcatel.fr) wrote:
: |> : Just a nit, but insertion and extraction functions should not
: |> : generally be friends.
: |> Agree, minimise friends.
: |> : If they are, they allow the user to get at the internal
: |> : representation.
: |> I don't see this. The user uses the non-member function which is
: |> part of the class interface. How does that give the user access to
: |> internals?
: Extract with ostrstream, modify the text, and insert with istrstream.
: OK, it's pretty sneaky, really Machiavelli.
And has nothing to do with the extraction and insertion operators being
friends. I can agree that making them friends widens the number of
functions in the public interface which must be evaluated when the
implementation in changed. As non-friends, they need not be considered
when changing only the implementation. Still nothing to do with user
access to internals.
: My point, however, is that
: what you output is part of the *external* interface; if something isn't
: accessible through the public interface, it probably should not be part
: of the data output to a stream.
I accept that, and only a friend function could do that. A good point
that should be considered when designing the i/o interface. Other
factors should control the choice of friendship. I think we agree.
John
Bill Wade
Herb Sutter <he...@cntc.com> wrote in article
<5fqhd5$5...@netlab.cs.rpi.edu>...
[deleted]
> You are doing a code review. A programmer has written the following
class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
>
> class Complex {
[deleted]
> void operator+ ( Complex other )
[deleted]
> private:
> double _real, _imaginary;
> };
Most of the replies, and the answer at Herb's web page contained statements
to the effect
"2. Style: For efficiency, the parameter should be a const&"
from the dogma "pass classes by reference rather than by value."
Is the dogma correct here?
If operator+ is actually inlined, the compiler has enough information to do
the best it can, and it could generate the same code for pass by value or
pass by reference.
If operator+ is not inlined the compiler will have to have its own rule
about how to pass structs with two reals by value. Presumably the rule
will be based on how the underlying system performs. One possibility is
that it passes them with the pointer mechanism we expect from const& (and
then does a local copy if the argument gets modified). In this case we
also break even. On machines with lots of floating point registers and
expensive memory access the compiler might decide to do pass by value by
sticking all of the members into registers. This can be a performance
plus. About the only way it can hurt performance is by requiring two
floating point registers. Call by reference code can implement the
function using only one floating point register.
class foo{ T a,b;};
foo operator+(foo a, foo b){ a.a += b.a; a.b += b.b; return a; }
vs.
foo operator+(const foo& a, const foo& b)
{
foo t = a; or foo t;
t.a += b.a; t.a = a.a + b.a;
t.b += b.b; t.b = a.b + b.b;
return t; return t;
}
The call by value code is faster, on my system, by 5-10% when T is double,
and faster by about 30% if T is int.
Call by value can be a plus if you need a new object anyway or if you're
going to access every member of the argument by value. "The exception
makes the rule." Classes which are small enough and have a default copy
constructor can benefit by being passed by value. If your compiler is
smart enough it can almost never hurt performance to pass a class by value
rather than by const reference (unless your algorithm needs the address of
the original object, perhaps because of mutable members) to a function
which will access all of the members anyway.
There are plenty of style reasons to pass by const reference. Many of them
are valid:-)
Tom Houlder
Herb Sutter wrote:
>
> You are doing a code review. A programmer has written the following class,
> which shows some poor style and has some real errors. How many can you
> find, and how would you fix them?
>
> class Complex {
-Snip-
>
> private:
> double _real, _imaginary;
> };
First of all, _real and _imaginary should be public.
(*cringe*, public data, NO!)
Hiding these data makes no sense, except that it's good looking. On
the other hand it prevents the client from freely manipulating the
real and the imaginary parts of the complex number.
If for instance the real part should be multiplied by 2, we must first
get the real part, then multiply it by 2 and finally store it, either
by using an accessor or by constructing a new complex object.
Alternatively we can first construct a 'complex(2, 1)' and then use
'complex::operator*(complex const&)'. Both approaches are unnatural
and costly.
The mathematical notion of complex numbers is simply based on two sets
of numbers together with some additional structure. From that follows
that _all_ the properties of the numbers (real or integers) are
preserved. This is not the case with the complex class in the
standard library or the one above which have removed most of the
properties of the data members. Treating a complex number as an
entity may please a programmer, but when it eventually comes down to
optimisation this violation of the underlying mathematics will
manifest itself.
If the mathematics had been at least thought of, maybe it would have
been possible to come up with a usable complex class in the std. lib.
Now, everyone must implement their own if they need to optimise it.
It's not sure it's possible to find a good compromise. Some would for
instance need to optimise on the magnitude, and would prefer that this
was calculated and stored each time one of the data was changed. This
would require accessor functions.
In brief, as it seems difficult to implement complex numbers generally
enough it should not be in the standard. IMO, it was a gross failure
which can be compared to what it would be if a quasi-generic Employee
class had been added.
Tom
Tom Houlder
thou...@icor.fr
Bradd W. Szonye
Tom Houlder <thou...@icor.fr> wrote in article
<5ggv7l$m...@netlab.cs.rpi.edu>...
> In brief, as it seems difficult to implement complex numbers generally
> enough it should not be in the standard. IMO, [including class complex
> in the C++ standard library] was a gross failure
> which can be compared to what it would be if a quasi-generic Employee
> class had been added.
Or by including a quasi-generic psuedo-random-number generator in the
standard (since applications that really need them need a custom one),
or
by including a quasi-generic length/data string class, or value-based
(as
opposed to intrusive or polymorphic) sequences, or tree-based
dictionaries...
There will be times when particular application domains will need to
customize classes which are already in the library. Chaos researchers
and
simulators can not use rand() out of the box; systems-programmers
interfacing to BASIC or other string-optimized languages cannot use
basic_string<> out of the box; compiler-writers might not be satisfied
with
tree-based symbol tables, and mathematicians might not entirely like
complex. There's always some people who get stepped on by a generic
library
class. However...
While it's fun to write your own string or complex (or even the whole
standard library), for the most part, I'd rather use a shrink-wrapped
class
unless I really need better or I really want to do it myself for fun.
It's
certainly better than going shopping for a 3rd-party library (not
necessarily compatible with the others) or spending a week writing and
debugging a complex class when really all I want to do is throw together
a
quick complex statistical model. Or if I'm writing (yet another) virtual
calculator.
In other words, if your application focuses heavily on a standard type,
it
makes sense to evaluate the standard implementation carefully and
replace
it if it's insufficient. In other cases, it's much more important to
have
*any* pre-written class that does the job easily. This isn't all that
different from profiling for performance, except in the amount of code
replaced.
--
Bradd W. Szonye
bra...@concentric.net
Tom Houlder
>>>>> "Bradd" == Bradd W Szonye <Nutty_I...@msn.com> writes:
Bradd> While it's fun to write your own string or complex (or even
Bradd> the whole standard library), for the most part, I'd rather
Bradd> use a shrink-wrapped class unless I really need better or I
Bradd> really want to do it myself for fun. It's certainly better
Bradd> than going shopping for a 3rd-party library (not
Bradd> necessarily compatible with the others) or spending a week
Bradd> writing and debugging a complex class when really all I
Bradd> want to do is throw together a quick complex statistical
Bradd> model. Or if I'm writing (yet another) virtual calculator.
The complex class is not intended for fun applications. It is in the
library to provide scientific programmers a common class for treating
complex numbers and was probably included since valarray was.
Valarray was included to avoid temporaries, and it is natural to
expect that complex is as fast as possible. If I for instance want to
add to the imaginary part of a complex number I can't do that without
also adding zero to the real part. You can't expect a mainstream
scientific programmer to write his own complex class, and when he sees
that Fortran is twice as fast for these operations, C++ is scrapped.
There is no such thing as "*A* complex number". What we in daily
speech call a complex number is two numbers. A complex class should
hence be regarded as a container class with two numbers, just like the
pair class. It is as simple as that.
Do you protect a one-dimensional variable with accessors? No. Do you
protect an N dimensional variable with accessors? No. (We have
`reference vector::operator[](size_type n)'.) So why should you
protect a two dimensional variable as is a complex number? There
simply is no reason except that library writers can optimize some of
the other member functions in the class which only will be of value to
a very few people. And I fully agree with Bradd that these people can
write their own class. However, the accessors "look good" to many
programmers and when something looks good the practical implications
are often forgotten. Letting programmers do mathematics is no better
than letting mathematicians program.
If the data in the complex class are made public, the class becomes
faster, it becomes consistent with the rest of the library and it
becomes mathematically consistent (for what that is worth). Said
another way, it will be used. As it is now, it can only be used if
you write "(yet another) virtual calculator".
Tom
Tom Houlder
thou...@icor.fr
Christian Millour
John E. Potter wrote:
> class Complex { [details snipped] }
> // #include consistent.lazyness.h
> template <class T>
> T operator+ ( T const& lhs, T const& rhs ) {
> return static_cast<T>(lhs) += rhs;
> // One of my dumb compilers sees T(lhs) as const_cast<T&>(lhs)
> }
I don't understand how this works in the case of Complex. According
to fragments of the dec96wp that seem relevant:
> 5.2.9 Static cast [expr.static.cast]
>
> 1 The result of the expression static_cast<T>(v) is the result of con-
> verting the expression v to type T. If T is a reference type, the
> result is an lvalue; otherwise, the result is an rvalue. Types shall
> not be defined in a static_cast. The static_cast operator shall not
> cast away constness (_expr.const.cast_).
>
> 2 An expression e can be explicitly converted to a type T using a
> static_cast of the form static_cast<T>(e) if the declaration T t(e);"
> is well-formed, for some invented temporary variable t (_dcl.init_).
> The effect of such an explicit conversion is the same as performing
> the declaration and initialization and then using the temporary vari-
> able as the result of the conversion. The result is an lvalue if T is
> a reference type (_dcl.ref_), and an rvalue otherwise. The expression
> e is used as an lvalue if and only if the initialization uses it as an
> lvalue.
>
IMHO, T is not a reference type, the result is an rvalue, and the code
above
shouldn't compile. Or has the rule changed again ?
David Vandevoorde
Tom Houlder wrote:
[...]
> If the data in the complex class are made public, the class becomes
> faster,
That is not accurate. With a good compiler, there is no
difference in speed between direct access of struct fields
and access through inline member functions.
See, e.g., Lipmann's object model book for a discussion.
> it becomes consistent with the rest of the library and it
> becomes mathematically consistent (for what that is worth). Said
> another way, it will be used. As it is now, it can only be used if
> you write "(yet another) virtual calculator".
Most of what you say is unfounded IMO (which library structures
not inherited from the standard C library have public data
members??). However, I do agree that it would have been beneficial
to the scientific programming community to guarantee that
complex<T> is layout-compatible with an aggregate
struct {
T re_, im_;
};
Daveed
Tom Houlder
>>>>> "DV" == David Vandevoorde <dav...@vandevoorde.com> writes:
DV> Tom Houlder wrote: [...]
>> If the data in the complex class are made public, the class
>> becomes faster,
DV> That is not accurate. With a good compiler, there is no
DV> difference in speed between direct access of struct fields and
DV> access through inline member functions.
Not so.
If `T real()' and `T imag()' had returned (non-constant) references
instead there would be no difference. As they don't, and as functions
like `set_real(T)' and `set_imag(T)' are not provided in complex<T>,
you have to first instantiate a new complex object and then perform an
operation on both data in order to just change one of the data.
For instance, to add the number n to the imaginary part of a complex
number c you have to either write
c = complex<T>(c.real(), c.imag() + n);
or
c += complex<T>(0, n);
instead of the natural and much faster
c.im += n;
This is simply not acceptable in time critical scientific applications
and complex<T> will not be used for these in its present
implementation.
>> it becomes consistent with the rest of the library and it
>> becomes mathematically consistent (for what that is worth).
>> Said another way, it will be used. As it is now, it can only
>> be used if you write "(yet another) virtual calculator".
DV> Most of what you say is unfounded IMO (which library
DV> structures not inherited from the standard C library have
DV> public data members??).
pair<T1, T2> is a close analog to complex<T>, and has public data
members as it should.
`T real()' and `T imag()' could be made to return references to the
corresponding data *if* one thinks this is more consistent with the
rest of the library. But that would hide what's going on and would
introduce unnecessary additional structure. There should be a reason
for everything in a library, also for hiding data. If there is a good
reason for making the data in complex<T> private it has at least
escaped me.
Tom
Tom Houlder
thou...@icor.fr
Tom Houlder
>>>>> "TH" == Tom Houlder <thou...@icor.fr> writes:
TH> As they don't, and as functions like `set_real(T)' and
TH> `set_imag(T)' are not provided in complex<T>, you have to
TH> first ===>instantiate<=== a new complex object and then
TH> perform an operation on both data in order to just change one
TH> of the data.
Initialize, that is.
David Vandevoorde
Tom Houlder wrote:
>
> >>>>> "DV" == David Vandevoorde <dav...@vandevoorde.com> writes:
>
> DV> Tom Houlder wrote: [...]
> >> If the data in the complex class are made public, the class
> >> becomes faster,
>
> DV> That is not accurate. With a good compiler, there is no
> DV> difference in speed between direct access of struct fields and
> DV> access through inline member functions.
>
> Not so.
>
> If `T real()' and `T imag()' had returned (non-constant) references
> instead there would be no difference.
My point was that there is no need to make your data members
public. How exactly you implement the inline member functions
was not part of taht.
Furthermore, for built-in types (all the types for which you
can compliantly instantiate a complex are built-in) you'll
find that some compilers produce faster code when you return
by value instead of by reference or direct access (apparently
because it eases the aliasing analysis). Usually, however, it
makes no difference (at high optimization levels, that is).
> If there is a good
> reason for making the data in complex<T> private it has at least
> escaped me.
For example, because it guarantees that:
complex<double> z;
ptrdiff_t d = f();
cout << *((double*)&z + d);
has unspecified behavior... thereby easing more aggressive
optimizations (e.g., by marking the fields with an
implementation specific #PRAGMA).
Daveed
Stein Somers
> Tom Houlder wrote:
> > If `T real()' and `T imag()' had returned (non-constant) references
> > instead there would be no difference.
David Vandevoorde replied:
> My point was that there is no need to make your data members
> public. How exactly you implement the inline member functions
> was not part of taht.
But if public accessor functions return non-const references,
these data members *are* public!?
Stein