Error while Using Templates in C++

Question

The template i have is:

template <class T>
class Shape {
T val,val_new;
public:
Shape(T initval)
{
   val=initval;
}
T get()
{
   return val;
}
void set (T newval)
{
   val_new = newval;
}
void copy()
{
   val= val_new;
}
};

The class to use this template is :

#include <iostream>
#include<math.h>
using namespace std;
class Rectangle
{
 private:
 Shape<TwoPoint> val;
 bool incr_called, decr_called, load_called;
 TwoPoint newval;
 public:
 Rectangle(TwoPoint i)
      : val (Shape<TwoPoint> (i)) {}     
 Shape<TwoPoint> read()
 {
   return val;
 }
 void load(TwoPoint n)
 {
   load_called=1;
   newval=n;
 }
 void increment()
 {
   incr_called=1;
 }
 void decrement()
 {
   decr_called=1;
 }
 void init()
 {
   incr_called=0;
   decr_called=0;
   load_called=0;
 }
 void actions()
 { 
   if (load_called)
       val.set(new TwoPoint(newval));
   if(incr_called && !decr_called)
       val.set((new TwoPoint(val.get())).plus(1));
   if(!incr_called && decr_called)
       val.set((new TwoPoint(val.get())).plus(-1));
 }     
 };

TwoPoint class is defined as:

 class TwoPoint
 {
   int width;
   int value;
   public:
    TwoPoint()
    {
      value=0;
      width=0;
     }
     TwoPoint(int v, int w)
     {
       value=v;
       width=w;
     }
     TwoPoint(const TwoPoint& t)
     {
       value= t.value;
       width= t.width;
     }
     int getWidth()
     {
       return width;
     }
     int getValue()
     {
       return value;
     }
     TwoPoint & plus(int newval)
     {
      value+=newval;
      return *this;
     }
    };

But there are errors:

In member function 'void Rectangle::actions()':
error: request for member 'plus' in '(((TwoPoint*)operator new(8u)), (((*<anonymous>)
<unknown operator> TwoPoint<T>::get() [with T=TwoPoint]()), <anonymous>))', which is of non-class type 'TwoPoint*'

There is another error:

In member function 'void Rectangle::actions()':
error: no pattern matching function for call to 'Shape<TwoPoint>::set(TwoPoint*)'
note: candidates are: void Shape<T>:: set<T> [with T=TwoPoint]

There are two more errors when i am doing similar operations in actions() due to same reason. Can somebody please explain these errors and how to improve them? Is there any way in which i can make code more efficient?

Answer 1

Shape::set takes its argument by value, but you're creating a value using new and passing a pointer. You should avoid using new unless you actually need a dynamic object; in which case, you need to make sure it's deleted when you've finished with it.

In this case, you just want to pass an object by value:

 void actions()
 { 
   if (load_called)
       val.set(newval);
   if(incr_called && !decr_called)
       val.set(val.get().plus(1));
   if(!incr_called && decr_called)
       val.set(val.get().plus(-1));
 }     

Is there any way in which i can make code more efficient?

Dynamic allocation is usually less efficient than using automatic objects - fixing the error has also removed a source of inefficiency.

Shape::set, and the constructor, could take their arguments by reference, and Shape::get could return a reference, to avoid unnecessary copying; although in practice the compiler will probably avoid those copies anyway. Also, the constructor could use an initialiser list, to initialise the member(s) directly, rather than default-initialising them and the reassigning them. Code like this might be marginally more efficient in some cases:

Shape(T const & initval)      // pass by reference
  : val(initval)              // use initialiser list
{}
T const & get()               // return by reference
{
   return val;
}
void set (T const & newval)   // pass by reference
{
   val_new = newval;
}

But in general, focus on making the code correct and readable, and on choosing efficient algorithms; only worry about small inefficiencies if they prove to be a bottleneck.

Answer 2

The new operator was the obvious culprit there. Using automatic variables solves memory leakage. Code is further improved when classes like Shape and TwoPoint can be passed as reference and not by a copied value. I admit to being a little bored and fiddling a bit with your code, even add debugging output with ostream. I cannot help you with the logic of the application though. I have no idea why certain constructs are there, so I kept most of them intact (except for val_new, because currently it added nothing to the code).

#include <iostream>
#include <math.h>
using namespace std;

class TwoPoint {
    int value, width;
public:
    TwoPoint() : value(0), width(0) {}
    TwoPoint(int v, int w) : value(v), width(w) {}
    TwoPoint(const TwoPoint& t) : value(t.value), width(t.width) {}
    int getWidth() { return width; }
    int getValue() { return value; }
    TwoPoint & plus(int newval) { value += newval; return *this; }
    friend ostream& operator<< (ostream& os, const TwoPoint& x);
};

template <class T> class Shape;

template <class T>
ostream& operator<< (ostream& os, const Shape<T>& x);

template <class T>
class Shape {
    T val;  // do you really need val_new?
public:
    Shape(T initval) : val(initval) {}
    T & get() { return val; }
    void set (T const & newval) { val = newval; }
    // not sure why you used and set val_new instead of val...
    friend ostream& operator<< <> (ostream& os, const Shape<T>& x);
};

class Rectangle {
private:
    Shape<TwoPoint> val;
    bool incr_called, decr_called, load_called;
    TwoPoint newval;
public:
    Rectangle(TwoPoint i) : val(Shape<TwoPoint> (i)),
        incr_called(false), decr_called(false), load_called(false) {}     
    Shape<TwoPoint> & read() { return val; }
    void load(const TwoPoint& n) { load_called = true; newval = n; }
    void increment() { incr_called = true; }
    void decrement() { decr_called = true; }
    void init() { incr_called = decr_called = load_called = 0; }
    void actions() {
        if (load_called) {
            val.set(TwoPoint(newval));
            load_called = false;  // should the flag be reset?
        }
        if(incr_called && !decr_called) {
            val.set(val.get().plus(1));
            incr_called = false;  // should the flag be reset?
        }
        if(!incr_called && decr_called) {
            val.set(val.get().plus(-1));
            decr_called = false;  // should the flag be reset?
        }
    }     
    friend ostream& operator<< (ostream& os, const Rectangle& x);
};

// added for debug printouts:
ostream& operator<< (ostream& os, const TwoPoint& x){
    os << "TwoPoint( " << x.value << ", " << x.width << " )";
    return os;
}
template <class T>
ostream& operator<< (ostream& os, const Shape<T>& x){
    os << "Shape( " << x.val << " )";
    return os;
}
ostream& operator<< (ostream& os, const Rectangle& x){
    os << "Rectangle( " << x.val 
    << (x.load_called ? ", load_called" : "")
    << (x.incr_called ? ", incr_called" : "")
    << (x.decr_called ? ", decr_called" : "")
    << " )";
    return os;
}

int main() {
    TwoPoint tp(800, 300);
    cout << "Creating a Rectangle using " << tp << endl;
    Rectangle r(tp);
    cout << r << endl;
    r.load(TwoPoint(100, 200));
    cout << r << endl;
    r.actions();
    cout << r << endl;
    r.increment();
    cout << r << endl;
    r.actions();
    cout << r << endl;
    r.decrement();
    cout << r << endl;
    r.actions();
    cout << r << endl;
    return 0;
}

And the output of the program:

Creating a Rectangle using TwoPoint( 800, 300 )
Rectangle( Shape( TwoPoint( 800, 300 ) ) )
Rectangle( Shape( TwoPoint( 800, 300 ) ), load_called )
Rectangle( Shape( TwoPoint( 100, 200 ) ) )
Rectangle( Shape( TwoPoint( 100, 200 ) ), incr_called )
Rectangle( Shape( TwoPoint( 101, 200 ) ) )
Rectangle( Shape( TwoPoint( 101, 200 ) ), decr_called )
Rectangle( Shape( TwoPoint( 100, 200 ) ) )

I hope this debug stuff proves useful when developing this app further.

Answer 3

You have this:

(new TwoPoint(val.get())).plus(1)

new returns a pointer-to-TwoPoint, so you have to use -> instead of . to access member functions.

But if you did that, you'd get a memory leak in the general case. I would suggest rethinking your design so that you don't need to dynamically allocate things.