Errors

[Latina]

Hi I am doing a program oveloraded operator.
I am having a few errors on it.

Error1: request for member `insertElement' in `S1set', which is of
non-
class type `IntegerSet[26]'

Error2: no matching function for call to `IntegerSet:perator+
(IntegerSet[26])'

Here is my program:


class IntegerSet
{
private:
bool set[26];
int element;

public:
//Operator methods.
IntegerSet operator + (const IntegerSet &)const;//method union

//Methods
IntegerSet(); //default constructor
IntegerSet(int x[], int k); //overload constructor

bool isValid(int)const;
void insertElement(int);
void deleteElement(int);
void setString();
void inputSet();
};


IntegerSet set();

IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}

IntegerSet::IntegerSet(int x[], int k)
{
for(element=0; element>=25; element++)
set[element]= false;
for(int j=0; j<k; j++)
{
element=x[j];
set[element]= true;
}
}

bool IntegerSet::isValid(int i)const
{
return set;
}

//insert element to a set
void IntegerSet::insertElement(int element)
{
set[element]=true;


}

//delete element of a set
void IntegerSet::deleteElement(int element)
{
set[element]=false;


}

//overloaded operator + to compute the union of two sets
IntegerSet IntegerSet:perator+(const IntegerSet &right)const
{
IntegerSet j;

for(int element=0; element<=25; element++)
{
if(isValid(element) || right.isValid(element))
j.insertElement(element);
}
return j;
}


int main()
{
IntegerSet run;
IntegerSet S1set[26];
IntegerSet S2set[26];
IntegerSet S3set[26];
IntegerSet Sset[26];

for(int i=2; i<=20; i+2)
S1set.insertElement(); <--Error 1

for(int k=6; k<=21; k+3)
S2set.insertElement(); <--Error 1

for(int j=3; j<=18; j+6)
S3set.insertElement(); <--Error 1

for(int z=0; z<=25; z++)
Sset.insertElement(); <--Error 1

run.inputSet();
int choice;

cout<<"\n WELCOME to the INTEGER SET PROGRAM\n";
cout<<"\n\nSelect one of these choices\n";
cout<<" 0. Create set \n";
cout<<" 1. Find Union \n";
cin>>choice;

if(choice==0)
{
int temp, ele;
int newSet[26];

cout<<"Enter how many elements you want in the set: "<<endl;
cin>>ele;

for(int i=0; i<ele; i++)
{
cout<<i+1;
cin>>temp;
newSet=temp;
}
}
else if(choice==1)
{
char a, b, c, d, e,;
int option;

cout<<"Select one of this choices";
cout<<"a. To find the union of the set you enter and the set
'S'";
cout<<"b. To find the union of the set you enter and the set
'S1'";
cout<<"c. To find the union of the set you enter and the set
'S2'";
cout<<"d. To find the union of the set you enter and the set
'S3'";
cin>>option;
if(option=='a'||option=='A')
{
run.operator+(Sset); <--Error 2
}
else if(option=='b'||option=='B')
{
run.operator+(S1set); <--Error 2
}
else if(option=='c'||option=='C')
{
run.operator+(S2set); <--Error 2
}
else if(option=='d'||option=='D')
{
run.operator+(S3set); <--Error 2
}
}

return 0;
}


I hope some one can help me.

 

[Alf P. Steinbach]

* Latina:

Hi I am doing a program oveloraded operator.
I am having a few errors on it.

Error1: request for member `insertElement' in `S1set', which is of
non-
class type `IntegerSet[26]'

This means that you have declared S1set as a raw array of 26 IntegerSet
elements, but you're trying to call a member function on the array. A
raw (built-in) array does not have member functions. Probably you meant
to declare S1Set as a single object of type IntegerSet.

Error2: no matching function for call to `IntegerSet:perator+
(IntegerSet[26])'
Ditto.


[snip]
int main()
{
IntegerSet run;
Good.


IntegerSet S1set[26];

Bad.


Cheers, & hth.,

- Alf

 

[Tadeusz B. Kopec]

Hi I am doing a program oveloraded operator. I am having a few errors on
it.

Quite many, i would say. First of all, if it's not an exercise someone
gave to you but a class to be used somewhere, you should use

class IntegerSet
{
private:
bool set[26];

First of all, it seems to be a set of integers from range [0..25]. Is it
what you wanted?

int element;

What is this member variable for? The only use I could see is a loop
variable and for that a local variable would do.

public:
//Operator methods.
IntegerSet operator + (const IntegerSet &)const;//method union

//Methods
IntegerSet(); //default constructor

This comment is useless - doesn't say anything more that code does.

IntegerSet(int x[], int k); //overload constructor

This comment is also useless. But this constructor needs a comment:
"x - array of integers to add to set
k - size of x
x must not contain numbers greater than 25 nor less then 0".

bool isValid(int)const;

// It should be rather "contains". There is nothing invalid in not being
kept in a specific set.

void insertElement(int);
void deleteElement(int);
void setString();
void inputSet();
};


IntegerSet set();

You declare a function named set. What for? You don't seem to use it.

IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}

The only thing this function does is assigning 0 to element.

IntegerSet::IntegerSet(int x[], int k) {
for(element=0; element>=25; element++)
set[element]= false;

Again you assign 0 to element and nothing else;

for(int j=0; j<k; j++)
{
element=x[j];
set[element]= true;

If x contains a number greater than 25 or less then 0, you have undefined
behaviour

}
}

bool IntegerSet::isValid(int i)const
{
return set;
}

Do you intentionally omit bound checking?

//insert element to a set
void IntegerSet::insertElement(int element) {
set[element]=true;
}

// Comments are useful at method declaration not definition. Again you
omit bound checking.

//delete element of a set
void IntegerSet::deleteElement(int element) {
set[element]=false;
} Ditto

//overloaded operator + to compute the union of two sets IntegerSet
IntegerSet:perator+(const IntegerSet &right)const {
IntegerSet j;

for(int element=0; element<=25; element++) {
if(isValid(element) || right.isValid(element))
j.insertElement(element);
}
return j;
}


int main()
{
IntegerSet run;
IntegerSet S1set[26];
IntegerSet S2set[26];
IntegerSet S3set[26];
IntegerSet Sset[26];

for(int i=2; i<=20; i+2)
S1set.insertElement(); <--Error 1

As Jim Langston noticed S1set is an array. On which element of it would
you like to invoke the method. And when you decide which one, shoudn't
you give an argument?

for(int k=6; k<=21; k+3)
S2set.insertElement(); <--Error 1

for(int j=3; j<=18; j+6)
S3set.insertElement(); <--Error 1

for(int z=0; z<=25; z++)
Sset.insertElement(); <--Error 1

run.inputSet();
int choice;

cout<<"\n WELCOME to the INTEGER SET PROGRAM\n";
cout<<"\n\nSelect one of these choices\n"; cout<<" 0. Create set
\n";
cout<<" 1. Find Union \n";
cin>>choice;

if(choice==0)
{
int temp, ele;
int newSet[26];

cout<<"Enter how many elements you want in the set: "<<endl;
cin>>ele;

What if user enters 'Hello world'? And what if '1024'?

for(int i=0; i<ele; i++)
{
cout<<i+1;
cin>>temp;
newSet=temp;
}
}
else if(choice==1)
{
char a, b, c, d, e,;
int option;

cout<<"Select one of this choices";
cout<<"a. To find the union of the set you enter and the set
'S'";
cout<<"b. To find the union of the set you enter and the set
'S1'";
cout<<"c. To find the union of the set you enter and the set
'S2'";
cout<<"d. To find the union of the set you enter and the set
'S3'";
cin>>option;
if(option=='a'||option=='A')
{
run.operator+(Sset); <--Error 2

Again Sset is an array.
Also you haven't done anything with variable run yet. Is it intended? And
the only sense of providing operator+ is to write 'run + Sset[0]' instead
of 'run.operator+(Sset[0])'

}
else if(option=='b'||option=='B')
{
run.operator+(S1set); <--Error 2
}
else if(option=='c'||option=='C')
{
run.operator+(S2set); <--Error 2
}
else if(option=='d'||option=='D')
{
run.operator+(S3set); <--Error 2
}
}

return 0;
}


I hope some one can help me.

HTH
Greetings

 

[James Kanze]

Quite many, i would say. First of all, if it's not an exercise someone
gave to you but a class to be used somewhere, you should use
std::set<int> or make a class that's just a wrapper for it. Your class
has many problems.

class IntegerSet
{
private:
bool set[26];

First of all, it seems to be a set of integers from range
[0..25]. Is it what you wanted?

It looks like it.

The number 26 is very suggestive. Perhaps what he really wants
is SetOfLetters.

Second - it's wasting space. Use std::vector<bool> or if you
want a fixed size - std::bitset.

As has often been pointed out, std::vector<bool> is broken, and
should be avoided. And it's likely to take up just as much, or
more space than the original code. (On my machine, I get
sizeof(bool) == 1, sizeof(std::vector<bool>) == 40. And that
doesn't count the memory dynamically allocated by std::vector.)

Of course, you can do better. My SetOfCharacter uses an array
of 32 bytes, for 256 possible characters. For 26, I'm not sure
it's worth it. For that matter, for 256, I'm not sure its worth
it, unless you have a lot of instances. (My implementation of
SetOfCharacter does this, but it was written back in the days
when I was using a 16 bit machine, with a maximum of 64 KB
memory.)

What is this member variable for? The only use I could see is a loop
variable and for that a local variable would do.
This comment is useless - doesn't say anything more that code does.

Constructors generally do need a comment concerning the post
conditions, however. Something to the order of "creates an
empty set", or:

//! \post

IntegerSet(int x[], int k); //overload constructor

This comment is also useless. But this constructor needs a comment:
"x - array of integers to add to set
k - size of x
x must not contain numbers greater than 25 nor less then 0".

In which case, a char[] would be just as good. And of
course, it should be "int const x[]" or "char const x[]".

If he really is implementing SetOfLetter, I'd use char const*.
Allowing anything, ignoring non-alpha, and converting all upper
and lower case to the corresponding integral value. But since
we don't know what the class is for (its role or its
responsibilities), we can't say.

bool isValid(int)const;

// It should be rather "contains". There is nothing invalid in not being
kept in a specific set.

void insertElement(int);
void deleteElement(int);
void setString();
void inputSet();
};
IntegerSet set();

You declare a function named set. What for? You don't seem to use it.

IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}

The only thing this function does is assigning 0 to element.

It initializes the set to empty. Seems like a reasonable thing
to do for a default constructor. (Of course, using std::fill
would be more idiomatic. But his code seems quite reasonable.)

IntegerSet::IntegerSet(int x[], int k) {
for(element=0; element>=25; element++)
set[element]= false;

Again you assign 0 to element and nothing else;

Again, initializing the set to empty.

for(int j=0; j<k; j++)
{
element=x[j];
set[element]= true;

If x contains a number greater than 25 or less then 0, you have undefined
behaviour

An assert would be in order, if that is the precondition.

}
}
bool IntegerSet::isValid(int i)const
{
return set;
}

Do you intentionally omit bound checking?

//insert element to a set
void IntegerSet::insertElement(int element) {
set[element]=true;
}

// Comments are useful at method declaration not definition. Again you
omit bound checking.

And a comment like his isn't useful at all. It doesn't say
anything more than the function name. A comment specifying the
pre-condition would be useful, however.

I'd also call this function in the non-default constructor.

//delete element of a set
void IntegerSet::deleteElement(int element) {
set[element]=false;
} Ditto

//overloaded operator + to compute the union of two sets IntegerSet
IntegerSet:perator+(const IntegerSet &right)const {
IntegerSet j;
for(int element=0; element<=25; element++) {
if(isValid(element) || right.isValid(element))
j.insertElement(element);

Since this is a member function:
j.set[ element ] = set[ element ] | right.set[ element ] ;
No if necessary.

I'd also use "result", rather than j, as the name. Names like
i, j, k should be reserved for loop indexes.

 

[Tadeusz B. Kopec]

Hi I am doing a program oveloraded operator. I am having a few errors
on it.
class IntegerSet
{
private:
bool set[26];

[snip]
Second - it's wasting space. Use std::vector<bool> or if you want a
fixed size - std::bitset.

As has often been pointed out, std::vector<bool> is broken, and should
be avoided. And it's likely to take up just as much, or more space than
the original code. (On my machine, I get sizeof(bool) == 1,
sizeof(std::vector<bool>) == 40. And that doesn't count the memory
dynamically allocated by std::vector.)

OK. I thought that vector<bool> not being a container isn't a problem for
the use that is made in this class, but anyway it sacrifices speed for
(probable) space gain and it's a premature optimisation (if it is an
optimisation).

[snip]

IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}

The only thing this function does is assigning 0 to element.

It initializes the set to empty. Seems like a reasonable thing to do
for a default constructor. (Of course, using std::fill would be more
idiomatic. But his code seems quite reasonable.)

It would do this, if the loop condition was 'element <= 25'. As it is,
only the assignment to element will be executed. And of course using a
member as a loop controlling variable is a bad idea.

 

[James Kanze]

On Sun, 11 Nov 2007 21:46:46 -0800, Latina wrote:
Hi I am doing a program oveloraded operator. I am having a few errors
on it.
class IntegerSet
{
private:
bool set[26]; [snip]
Second - it's wasting space. Use std::vector<bool> or if you want a
fixed size - std::bitset.

As has often been pointed out, std::vector<bool> is broken, and should
be avoided. And it's likely to take up just as much, or more space than
the original code. (On my machine, I get sizeof(bool) == 1,
sizeof(std::vector<bool>) == 40. And that doesn't count the memory
dynamically allocated by std::vector.)

OK. I thought that vector<bool> not being a container isn't a problem for
the use that is made in this class, but anyway it sacrifices speed for
(probable) space gain and it's a premature optimisation (if it is an
optimisation).

I don't think it would be an actual problem, but as a matter of
principle, I think it better to avoid vector<bool>, because it
looks like a container, even if it isn't one. Learning all its
quirks, to be sure of what you are doing, is probably more work
than just using the C style array (in this case, at least).

[snip]

IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}
The only thing this function does is assigning 0 to element.

It initializes the set to empty. Seems like a reasonable thing to do
for a default constructor. (Of course, using std::fill would be more
idiomatic. But his code seems quite reasonable.)

It would do this, if the loop condition was 'element <= 25'.

Oops. But that's obviously a typo.

As it is, only the assignment to element will be executed. And
of course using a member as a loop controlling variable is a
bad idea.

The overall structure of the code wasn't particularly good,
agreed. As I said, I'd use std::fill here. But the principle
of a loop isn't necessarily wrong, even if he miswrote it, and
didn't do it very cleanly.