1

I am currently in a collage second level programing course... We are working on operator overloading... to do this we are to rebuild the vector class... I was building the class and found that most of it is based on the [] operator. When I was trying to implement the + operator I run into a weird error that my professor has not seen before (apparently since the class switched IDE's from MinGW to VS express...) (I am using Visual Studio Express 2008 C++ edition...)

Vector.h

#include <string>
#include <iostream>
using namespace std;

#ifndef _VECTOR_H
#define _VECTOR_H

const int DEFAULT_VECTOR_SIZE = 5;

class Vector
{
private:
    int *   data;
    int     size;
    int     comp;
public:
    inline  Vector      (int Comp = 5,int Size = 0) 
        : comp(Comp), size(Size)    { if (comp > 0) { data = new int [comp]; } 
                                      else { data = new int [DEFAULT_VECTOR_SIZE];
                                      comp = DEFAULT_VECTOR_SIZE; }
                                    }
    int      size_      ()          const       { return size; }
    int      comp_      ()          const       { return comp; }
    bool     push_back  (int);
    bool     push_front (int);
    void     expand     ();
    void     expand     (int);
    void     clear      ();
    const    string at  (int);
    int&         operator[ ](int);
    int&         operator[ ](int) const;
    Vector&  operator+  (Vector&);
    Vector&  operator-  (const Vector&);
    bool     operator== (const Vector&);
    bool     operator!= (const Vector&);

    ~Vector() { delete [] data; }
};

ostream& operator<< (ostream&, const Vector&);

#endif

Vector.cpp

#include <iostream>
#include <string>
#include "Vector.h"
using namespace std;

const string Vector::at(int i) {
    this[i];
}

void Vector::expand() {
    expand(size);
}

void Vector::expand(int n ) {
    int * newdata = new int [comp * 2];
    if (*data != NULL) {
        for (int i = 0; i <= (comp); i++) {
            newdata[i] = data[i];
        }
        newdata -= comp;
        comp += n;
        data = newdata;
    delete newdata;
    }
    else if ( *data == NULL || comp == 0) {
        data = new int [DEFAULT_VECTOR_SIZE];
        comp = DEFAULT_VECTOR_SIZE;
        size = 0;
    }
}

bool Vector::push_back(int n) {
    if (comp = 0) { expand(); }
    for (int k = 0; k != 2; k++) {
        if ( size != comp ){
            data[size] = n;
            size++;
            return true;
        }
        else {
            expand();
        }
    }
    return false;
}

void Vector::clear() {
    delete [] data;
    comp = 0;
    size = 0;
}
int& Vector::operator[] (int place) { return (data[place]); }
int& Vector::operator[] (int place) const { return (data[place]); }

Vector& Vector::operator+ (Vector& n) {
    int temp_int = 0;

    if (size > n.size_() || size == n.size_()) { temp_int = size; }
    else if (size < n.size_()) { temp_int = n.size_();  }

    Vector newone(temp_int);
    int temp_2_int = 0;

    for ( int j = 0; j <= temp_int && 
                     j <= n.size_() && 
                     j <= size; 
                                        j++) {
        temp_2_int = n[j] + data[j];
        newone[j] = temp_2_int;
    }
////////////////////////////////////////////////////////////
    return newone;
////////////////////////////////////////////////////////////
}

ostream& operator<< (ostream& out, const Vector& n) {
    for (int i = 0; i <= n.size_(); i++) {
////////////////////////////////////////////////////////////
        out << n[i] << " ";
////////////////////////////////////////////////////////////
    }
    return out;
}

Errors:

out << n[i] << " "; error C2678:

binary '[' : no operator found which takes a left-hand operand of type 'const Vector' (or there is no acceptable conversion)

return newone;

error C2106: '=' : left operand must be l-value

As stated above, I am a student going into Computer Science as my selected major I would appreciate tips, pointers, and better ways to do stuff :D

Improve this question
4
  • The lines you are showing are not the ones that are causing the error messages. Also, is this supposed to be a vector of strings or of integers? Commented Apr 2, 2010 at 16:32
  • 1
    *data = *newdata;, as @Neil points out in his answer, is wrong: you almost certainly mean data = newdata;. Commented Apr 2, 2010 at 16:38
  • You got an awful lot of help here that's specific to this assignment beyond just diagnosing your error. Hopefully it's okay with your professor and school to do that sort of thing - if not, you might want to avoid that sort of general help request in the future. Commented Apr 2, 2010 at 17:40
  • @Jefromi: I posted this on his request - a good point for others reading this though Commented Apr 2, 2010 at 19:40

6 Answers 6

Reset to default
10

This:

int operator[ ](int);

is a non-const member function. It means that it cannot be called on a const Vector.

Usually, the subscript operator is implemented such that it returns a reference (if you return a value, like you are doing, you can't use it as an lvalue, e.g. you can't do newone[j] = temp_2_int; like you have in your code):

int& operator[](int);

In order to be able to call it on a const object, you should also provide a const version of the member function:

const int& operator[](int) const;

Since you ask for "tips, pointers, and better ways to do stuff:"

  • You cannot name your include guard _VECTOR_H. Names beginning with an underscore followed by a capital letter are reserved for the implementation. There are a lot of rules about underscores.
  • You should never use using namespace std in a header.
  • Your operator+ should take a const Vector& since it is not going to modify its argument.
  • Your at should return an int and should match the semantics of the C++ standard library containers (i.e., it should throw an exception if i is out of bounds. You need to use (*this)[i] to call your overloaded operator[].
  • You need to learn what the * operator does. In several places you've confused pointers and the objects to which they point.
  • Watch out for confusing = with == (e.g. in if (comp = 0)). The compiler will warn you about this. Don't ignore warnings.
  • Your logic will be much simpler if you guarantee that data is never NULL.
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

What is the difference between putting the const before vs after the function declaration?
@Wallter: A const qualifier next to the return type const qualifies the return type (i.e., in this example, you return a reference to a const int). A const qualifier at the end of the function declaration const qualifies the member function (i.e., the this pointer in the member function is const qualified and you can't change any non-mutable members variables of the object or call any non-const member functions).
the const before makes the int& constant. the const after means that the method operator[]() doesn't change anything in the object that it acts on.
I would replace the return type const int& with int.
Just to clarify, the reason to never using namespace std (or anything) in a header is that including that header in any source file causes symbols to be unexpectedly brought into the global namespace of the includer and can then cause various problems.
|
3

Can't fit this into a comment on Neil's answer, so I'm gonna have to go into more detail here.

Regarding your expand() function. It looks like this function's job is to expand the internal storage, which has comp elements, by n elements, while maintaining the size of the Vector. So let's walk through what you have.

void Vector::expand(int n) {
    int * newdata = new int [comp * 2];

Okay, you just created a new array that is twice as big as the old one. Error: Why doesn't the new size have anything to do with n?

    if (*data != NULL) {

Error: *data is the first int element in your array. It's not a pointer. Why is it being compared to NULL?

Concept Error: Even if you said if (data != NULL), which could be a test to see if there is an array at all, at what point in time is data ever set to NULL? new [] doesn't return NULL if it's out of memory; it throws an exception.

        for (int i = 0; i <= (comp); i++) {
            newdata[i] = data[i];
        }

Warning: You're copying the whole array, but only the first size elements are valid. The loop could just run up to size and you'd be fine.

        newdata -= comp;

Error: Bad pointer math. newdata is set to a pointer to who knows where (comp ints back from the start of newdata?!), and almost certainly a pointer that will corrupt memory if given to delete [].

        comp += n;

This is fine, for what it is.

        data = newdata;
        delete newdata;
    }

Error: You stored a pointer and then immediately deleted its memory, making it an invalid pointer.

    else if ( *data == NULL || comp == 0) {
        data = new int [DEFAULT_VECTOR_SIZE];
        comp = DEFAULT_VECTOR_SIZE;
        size = 0;
    }
}

Error: This should be in your constructor, not here. Again, nothing ever sets data to NULL, and *data is an int, not a pointer.

What this function should do:

  • create a new array of comp + n elements
  • copy size elements from the old array to the new one
  • delete the old array
  • set data to point to the new array

Good luck.

Improve this answer

Comments

2

Besides of what others already wrote about your operator[]():

Your operator+() takes the right-hand side per non-const reference - as if it would attempt to change it. However, with A+B everyone would expect B to remain unchanged.

Further, I would implement all binary operators treating their operands equally (i.e., not changing either of them) as non-member functions. As member functions the left-hand side (this) could be treated differently. (For example, it could be subjected to overwritten versions in derived classes.)

Then, IME it's always good to base operator+() on operator+=(). operator+=() does not treat its operands equally (it changes its left one), so it's best done as a member function. Once this is done, implementing operator+() on top of it is a piece of cake.

Finally, operator+() should never, never ever return a reference to an object. When you say A+B you expect this to return a new object, not to change some existing object and return a reference to that.

Improve this answer

Comments

1

There are so many errors in your code that it is hard to know where to start. Here's one:

 delete [] data;
 *data = *newdata;

You delete a pointer and then immediately dereference it.

And:

const string Vector::at(int i) {
    this[i];
}

This is (I think) a vector of ints. why is this returning a string? And applying the [] operator to this does not call your operator[] overload - it treats this as an array, which it isn't.

Improve this answer

7 Comments

We are required to write the class in response to the teacher's driver.cpp I was under the impression that once i called the delete [] data; it would delete the data pointed to by *data - Is there a better way to expand the array (to gain the functionality of the traditional <vector> class?)
@Walter Once you delete a pointer, you cannot dereference it. This is a logic error in your code, nothing to do with your teacher's driver program.
Might want to point out that the right way to write at() would be return (*this)[i];.
@Mike No, it isn't, because as I pointed out this is an array of ints!
@Walter: To expand an array without discarding what is there, you need to create a new array, copy elements over from the old array, initialize the values in the expanded area (unless you just want to use their default constructor), then delete the old array and set data to point to the new array. In that order.
|
1

You need to provide two versions of your operator[]. For accessing:

T operator[](std::size_t idx)const;

For writing to the element:

T& operator[](std::size_t idx);

In both of the above, replace T with the type of the elements. The reason you have this problem is that only functions that are marked "const" may be invoked on an object declared to be "const". Marking all non-mutating functions as "const" is definitely something you should do and is called "const-correctness". Since returning a reference to an element (necessary for writing) allows the underlying object to be mutated, that version of the function cannot be made "const". Therefore, a read-only "const" overload is needed.

You may also be interested in reading:

Improve this answer

4 Comments

The first one really needs to be const T operator[](size_t idx) const. Otherwise a type T with reference members can still be modified.
@sblom, I am assuming that T will not alias the object in a modifiable manner, which I think is a reasonable assumption, but your criticism is nevertheless a valid one.
@sblom: In fact, since you don't know what T users might push onto your code, it might be best to return const T& from the const version of operator[](). So long as it's only built-ins, returning a copy is fine (and I don't see much benefit in making this const), but for user-defined types (say, a vector fo strings) that could be expensive.
There is no difference whatsoever between int function() and const int function(), because for scalar types like int, there is no such thing as a constant rvalue. See §3.10 section 9
0 
int Vector::operator[] (int place) { return (data[place]); }

This should be

int Vector::operator[] (int place) const { return (data[place]); }

so that you will be able to do the [] operation on const vectors. The const after the function declaration means that the class instance (this) is treated as const Vector, meaning you won't be able to modify normal attributes. Or in other words: A method that only has read access to attributes.

Improve this answer

2 Comments

from the comments below... once this is added the errors go away... but do i need a function declaration with out the const in it?
@Wallter: Sure, the function I described in my answer is only for read access when using a const Vector instance. If you want to assign values in the vector, like v[0] = 5;, you'll have to implement a non-const equivalent which returns a reference which can be assigned to.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.