Journal Articles
Browse in : |
All
> Journals
> CVu
> 145
(10)
All > Journal Columns > Code Critique (70) Any of these categories - All of these categories |
Note: when you create a new publication type, the articles module will automatically use the templates user-display-[publicationtype].xt and user-summary-[publicationtype].xt. If those templates do not exist when you try to preview or display a new article, you'll get this warning :-) Please place your own templates in themes/yourtheme/modules/articles . The templates will get the extension .xt there.
Title: Student Code Critique Competition
Author: Administrator
Date: 06 October 2002 13:15:55 +01:00 or Sun, 06 October 2002 13:15:55 +01:00
Summary:
Body:
Prizes provided by Blackwells Bookshops & Addison-Wesley
Please note that participation in this competition is open to all members. The title reflects the fact that the code used is normally provided by a student as part of their course work.
Note that this item is part of the Dialogue section of C Vu which is intended to designate it as an item where reader interaction is particularly important. Readers' comments and criticisms of published entries are always welcome.
Last time I asked for a critique of the following short program.
template <int N> class T { public: friend T operator+(const T&, const T&); private data[N + 1]; }; template <int N> T<N> operator+(const T<N>& S1, const T<N>& S2) { return S1; } int main(){ T<64> a, b, c = a + b; }
The critique was to include more than just identification and correction of errors. I wanted to see positive advice to the student on how better to achieve the objective. When I set the problem I realised that it would actually be rather more demanding than some of the earlier critiques. That I was right in this is illustrated by the fact that I only received three submissions. It is a pity that only one in four hundred members can manage to submit a solution. Yes, I know about time, but this was summer for most of you. Perhaps lying around on sun washed beaches is more attractive. Interestingly the three entries all come from outside the UK (India, US and Norway) and those members have less time to enter than those in the UK.
From Gurusami Annamalai <d0253028@ncb.ernet.in>
The first question that needs to be asked is "Is there a need to make the class T a template?" This question is important because, when we make a class a template, and there are too many instances of the template, then the space complexity of the resultant object code would increase. Paraphrased, improper usage of template class would lead to code bloat.
There is a more fundamental issue here. For example, in the above program, if the template parameter, specifies only the attributes of objects, rather than their behaviour, then its better made a member variable of the class, and the value could be assigned in the constructor. This means that, if
T<25> t1; T<50> t2;
behave the same (only their state differ), then the usage of templates is not justified.
I'll assume that class T being a template is justified and proceed further.
When we specify an access modifier, like public: etc, it doesn't apply to one member of the class. Rather it applies to all the members of the class that are declared after the modifier, either till the end of the class declaration, or the occurrence of another access modifier, whichever is earlier. Because of this semantics, it would be more self-descriptive if the access modifier is specified in a line by itself, followed by the members which it qualifies.
For example, it is better to write
public: friend T operator+(const T&, const T&);
than
public: friend T operator+(const T&, const T&);
The latter somehow gives the feeling that it applies only to the member along which it appears; the former, is self documenting. It seems to say that the following members are public. As a bonus, the former style helps to avoid errors like the one in the program, (reproduced below)
private data[N + 1]; // data type missing
While overloading an operator (like `+') for a user-defined data type, it is better kept as a member function of that class, unless the left operand of the operator being overloaded is of a different class. In that case, a friend function would be used.
In the above program we have
T operator+ (const T&, const T&); // friend function
Since the left hand operand of the operator (the first parameter) is a reference to an object of the class, for which we are overloading the operator, this function is best made a member function of the relevant class (here it is T). So we should prefer the following in place of the above.
T operator+ (const T&); // member function
Suppose we want to do something like
T<25> p; int j; T<25> r = j + p;
then we would need,
T<N> operator+(const int&, const T<N>&);
which cannot be converted to be used as a member of template class T. Only in such cases friend functions (to overload operators) are to be used.
The classic example to demonstrate this requirement is the overloading of operator<< to work with the ostream object cout. Of course, there are other examples, but I have given one.
The declaration,
friend T operator+(const T&, const T&);
taking account of its context in the above program, is the same as
friend T<N> operator+(const T<N>&, const T<N>&);
This, in spite of our best intentions, doesn't make any friends. This is because we are looking for a non-template function with name "operator+", which doesn't exist. (g++ compiler, version 2.96, warned me of this!
The above declaration should be
friend T<N> operator+<N>(const T<N>&, const T<N>&);
which can be simplified as, again taking account of its context in the above program,
friend T operator+<N>(const T&, const T&);
and further to,
friend T operator+<>(const T&, const T&);
By fixing the above program we get,
template<int N> class T { public: friend T operator+<> (const T&, const T&); private: int data[N + 1]; // int or anything }; template <int N> T<N> operator+(const T<N>& S1, const T<N>&S2){ return S1; } int main() { T<64> a, b, c = a + b; }
The better way to write the above code is,
template<int N> class T { public: T operator+(const T&); // member function private: int data[N + 1]; }; template<int N> T<N> T<N>::operator+ (const T<N>& S1){ return *this; // first parameter in given program } int main() { T<64> a, b, c = a+b; }
From Christopher Currie <christopher@currie.com>
Though the goal was to comment on C++ idioms, there are some syntax errors that will need to be corrected first, before the code can even be compiled. The original code, as printed, with line numbers added for clarity:
1 template <int N> 2 class T { 3 public: friend T operator+(const T&, 4 const T&); 5 private data[N + 1]; 6 }; 7 template <int N> 8 T<N> operator+ (const T<N>& S1, 9 const T<N>& S2) { 10 return S1; 11 } 12 int main() { 13 T<64> a, b, c = a + b; 14 }
The syntax errors are in the declaration of data; the omission of a colon after private and the lack of a data type. Access control keywords like private and public are not to be confused with type modifiers such as const. They are labels that affect the access of all the declarations that follow them. For this reason, it is good style to keep them on lines of their own:
1 template <int N> 2 class T { 3 public: 4 friend T operator+(const T&, 5 const T&); 6 private: 7 data[N + 1]; 8 };
Now we can clearly see the other problem with data, the lack of a data type. Legacy C compilers allowed identifiers to be implicitly int in the absence of a type declaration. Although some C++ compilers allow it, this is not valid C++.
7 int data[N + 1];
Now the code will compile, but it will not link. The reason is subtle, and the answer in this case covers many points of style that code borrows from textbook examples. As we clean up the style of the example, hopefully the answer will become clear.
A major source of confusion in the class is the use of T as an identifier. T is (over)used in books on template programming, most commonly to represent the type of the template parameter. In this case, the template parameter is not a type, it is an integral value. This illustrates the importance of using parameter names and class names that represent the purpose of the class or parameter. In this example, there is not enough context to know the purpose of this class, so we'll use the name FixedBuffer for the class, and length for the template parameter.
template <size_t length> class FixedBuffer { public: friend FixedBuffer operator+( const FixedBuffer&, const FixedBuffer&); private: int data[length + 1]; }; template <size_t length> FixedBuffer<length> operator+( const FixedBuffer<length>& S1, const FixedBuffer<length>& S2){ return S1; } int main() { FixedBuffer<64> a, b, c = a + b; }
This looks better, but now that the obscuring type names are gone, you might notice that the two operator+ functions look a little different...
From the C++ specification:
""When a template is instantiated, the names of its friends are treated as if the specialization had been explicitly declared at its point of instantiation.""
In our code, the function is treated as if it was declared as it appears in the friend function declaration. This draws attention to what the friend function is saying. As is common in template classes, the compiler does not require that you include the template parameters when using the class name within its declaration. The precise declaration would read:
template <size_t length> class FixedBuffer { public: friend FixedBuffer<length> operator+( const FixedBuffer<length>& S1, const FixedBuffer<length>& S1); ... };
But since it is within a template declaration, the name lookup doesn't happen until the class template is instantiated. When our class declared in main, it's template declares a friend function that looks like:
FixedBuffer<64> operator+( const FixedBuffer<64>& S1, const FixedBuffer<64>& S1);
Aha! This is not a template function! This is why the program will not link, for the signature of this function does not match the template function that is defined later in the code. In this case, one correct declaration would be:
template <size_t length> class FixedBuffer { public: friend FixedBuffer operator+<length>( const FixedBuffer& S1, const FixedBuffer& S1); ... };
(There is another syntax that my compiler accepts that requires that you declare the function before defining the class. I prefer the above.) This designates operator+ as a template function, and shows that the concrete instance of the template function that has a matching length template parameter is the actual friend.
Wow, all these rules for template friends of template classes are hard! Fortunately, there is a better way. The important point to realize is that operator+ doesn't have to be a friend! Think about the following:
int a, b, c; // ... assign values a = b + c;
Are b and c any different after the assignment than before? Of course not. The calculated result doesn't affect either of its parameters. In fact, we can't change either parameter, because its arguments are (as in this case) almost always const! The few operations that need rights to change an operand include the assignment operators and their kin, and these are almost always member functions that don't need to be friends anyway.
template <size_t length> class FixedBuffer { public: FixedBuffer& operator+=( const FixedBuffer& rhs){ // ...add rhs to *this return *this; } ... };
This code illustrates the canonical definition of the add-and-assign operator that takes its right hand argument as an operator, and returns a reference to itself, allowing the result to be used as an r-value (in other words, you can add the result to other variables, use it as a function argument, etc.). We've contained the code for adding FixedBuffer types within a member function, without any complicated friend functions. Now, instead of duplicating that code in operator+, we'll simply reuse it in the canonical definition of addition:
template <size_t length> FixedBuffer<length> operator+( const FixedBuffer<length>& S1, const FixefBuffer<length>& S2){ FixedBuffer<length> temp(S1); temp += S2; return temp; }
This very neatly allows us to change the definition of operator+=, and get an updated definition of operator+ absolutely free. This is typically the way most of the arithmetical operators are defined for classes that need addition and subtraction to work the way that we expect that integers do. Remember this need not only apply to template classes; regular classes can benefit just as much from this technique.
This deceptively small piece of code contained a lot of complexity, and in our analysis we can deduce a couple general rules. Remember, like all rules there are exceptions, but these will give the student place from which to build experience:
-
When a function needs to modify private or protected data within a class, make that function a member function if at all possible.
-
Define non-member, non-friend functions in terms of member functions to maximize maintainability and correctness.
From Terje Slettebo <tslettebo@chello.no>
I've been waiting for a C++ entry, and finally, one came. :)
Also, since I unwittingly "submitted" a blooper (which ended up as "Problem 3" in "Francis' Scribbles", C Vu June & August, :) ) let me try to "remedy" that by submitting this entry. By the way, in the following discussion, I agreed with Francis, about the problems with that code. It was originally intended to be just a test of a calculation, as well, not a well- tested routine. (Indeed you did, it just made a good problem for my column because it gave me an excuse to write about something I wanted to write about anyway. I am like that, scavenge for bits wherever they may be found.)
Regarding the given program. It says it can be compiled, but not linked. However, there are syntax error that makes it not even compile. These may be typos resulting from transferring the program to the magazine, though.
There are quite a few problems with this code, so let me take them in order:
-
Missing ":" after private.
-
Missing type of data, int chosen arbitrarily to fix it.
-
The operator+() doesn't make much sense, as it just returns one of the operands.
-
More seriously, the friend declaration declares a function, not a function template.
Point 4 leads to the following problem: When it comes to c=a+b it will have seen the following declarations:
T<64> operator+(const T<64> &,const T<64> &); // friend declaration only, instantiated // by the T<64>. template<int N> T<N> operator+(const T<N> &,const T<N> &); // Declaration and definition
Since the first is an exact match for c=a+b;, it won't instantiate the template, and you get a link error.
The fix is easy enough: Make the friend declaration a friend template declaration. One also needs to provide a reasonable operator+() implementation:
template<int N> class T { public: template<int M> friend T<M> operator+(const T<M> &, const T<M> &); private: int data[N+1]; }; template<int N> T<N> operator+(const T<N> &S1,const T<N> &S2){ T<N> temp; for(int i=0;i!=N;++i) temp.data[i]=S1.data[i]+S2.data[i]; return temp; }
As another comment, in general, "T" doesn't appear to be a very meaningful class name, either, but it's hard to tell, without knowing the context.
OK. On to the alternative. The article said "I want suggestions for coding idioms that will make the student's life easier," so let's see what we can do. A common idiom is to implement an operator in terms of the assignment-version of the operator. See for example [more-effective-cpp].
In the case above, operator+() could be a member function, but if you have an operator where the left-hand argument is another type, then it has to be a global function. So let's design a class for this general case, making the operator a global function. We'll also make a sensible operator+=() implementation, that operator+() will use. The compiler-generated copy constructor does the right thing, so none is provided, and other constructors are omitted, for brevity, and since they didn't appear in the original:
template<int N> class T { public: T &operator+=(const T &other){ for(int i=0;i!=N;++i) data[i]+=other.data[i]; return *this; } private: int data[N+1]; }; template<int N> T<N> operator+(const T<N> &S1,const T<N> &S2){ return T<N>(S1)+=S2; } int main() { T<64> a,b,c=a+b; }
This has a number of advantages over the first one:
-
By providing both += and +, you let the user of the class decide which to use, knowing that the former is generally more efficient, as it avoids creating a temporary object.
-
By implementing + in terms of +=, you ensure a consistent behaviour (and expected efficiency) for the two operators. The operation only needs to be implemented in +=, and you essentially get + "for free."
-
An important point is that you don't need any friend declaration, unlike the first version. operator+() needs no special access to T, as it only uses the public interface.
-
By implementing the operator+() as a general template, you may actually use it for several classes. Here, we only implement it for the class T, though.
Some details about the implementation:
-
As given in [more-effective-cpp], by using an unnamed temporary in the function template (rather than T<N> temp(S1); S1+=S2; return temp;), we make it possible for the compiler to perform the return value optimisation (eliminating the creation of any temporaries, by constructing the result at the call site), which may be available in more compilers, than the more recent named return value optimisation.
-
It may be debated if operator+=() should return T& or const T &. However, the advice in [effective-cpp] and precedence in the standard is to use T&, so that's what is used here. The rationale is basically to "do as the ints do."
I have taken particular care not to introduce typos into this piece and I have quoted the first part of a twenty-line error message, not least because it is less than helpful. The specific problem is one that you either see straight away or that will take you an embarrassingly long time to identify. However there are a number of other points that should be spotted and commented on. Remember you should never allow your students to go away with no more than the instant fix they seek.
I am trying to compile the following code and getting error that I don't understand.
In file pgsimeta.h
#include <vector> namespace PGSIMeta { class PgsiMeta { public: PgsiMeta(); virtual ~PgsiMeta(); bool operator==(const PgsiMeta); private: // MetaData is a class defined at the top typedef vector<MetaData> DataList; DataList dataList; }; }
In file pgsimeta.cp
#include "pgsimeta.h" using PGSIMeta; bool PgsiMeta::operator==( const PgsiMeta& obj){ return dataList == obj.dataList; }
Here is the error compiling:
_pgsi_meta.h:51: PGSIMeta::operator== (const PGSIMeta::PgsiMeta &)' must take exactly two arguments
Notes:
More fields may be available via dynamicdata ..