0

this question might have been answered before, but after two days of searching I couldn't find a solution.

I made a stack class which stores __data:

class __data
{
private:
    void* __p;
    __datatype __type;

public:
    __data(void);
    __data(int i);
    __data(double d);
    __data(char* s);
    __data(void (*f)(Stack));

    bool IsFunction(void);
    bool IsInteger(void);
    bool IsNumber(void);
    bool IsString(void);

    void* Get(void);
};

I created this class because the stack will be able to store strings, functions (pointers to functions), doubles and integers.

The problem is when I push an integer or double into the stack and then pop it and get the pointer to the data (void* Get(void)) I'm debugging it by printing out it's value, therefore I basically have this:

void print(Stack s)
{
    __data d = s.Pop();

    if (d.IsNumber()) // BUG: never the number I pass to __data
        std::cout << "IsNumber - " << *((double*)d.Get()) << std::endl;
    else if (d.IsInteger()) // BUG: it always prints out 1
        std::cout << "IsInteger - " << *((int*)d.Get()) << std::endl; // 
    else if (d.IsString())
        std::cout << "IsString - " << (char*)d.Get() << std::endl;
    else if (d.IsFunction())
    {
        std::cout << "IsFunction - " << d.Get() << std::endl;
        ((void (*)(Stack))d.Get())(s); // calls the function
    }
}

I don't know what might be wrong, maybe it's the way I assign __p (the pointer returned by Get()), these are the __data constructors for integer and double:

__data::__data(int i)
{
    __type = _dt_integer;
    __p = &i;
}
__data::__data(double d)
{
    __type = _dt_number;
    __p = &d;
}

So basically my problem is when I try to get the integer or double pointed by the returned void* it either gives me a value of 1 (integer) or 2.13171e-314 (double).

Thank you for your time, and sorry if there's already an answer for this out there.

Edit:

I'll re-write the Stack class and use a union instead. This seems to be the best method to achieve my goal.

zeluisping
  • 213
  • 7
  • 17
  • Why do you add emphasis to the classes names? – Blood Jul 16 '12 at 13:03
  • 6
    By the way, you shouldn't use those names (beginning with 2 _s) http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier – BoBTFish Jul 16 '12 at 13:04
  • Why use any pointers at all? Just use a `union` (and don't forget to duplicate any string, and delete the duplication in the destructor). – James Kanze Jul 16 '12 at 13:13

5 Answers5

1
__data::__data(int i)
{
    __type = _dt_integer;
    __p = &i; // this only stores the address of i on the stack
}

What you need is

__data::__data(int* i) // have the caller pass the address of their i
{
    __type = _dt_integer;
    __p = i; // this stores what you need
}
RedX
  • 14,749
  • 1
  • 53
  • 76
1

Your problem is that you save the address of a temporary object. E.g: in this example you save the address of the temporary double d.

__data::__data(double d)
{
    __type = _dt_number;
    __p = &d;
}

The object only exists in the current scope (the function) and the address is useless afterwards, as it may well be overridden by something other in the stack.

My proposed solution would be to make a dynamically allocated copy:

__data::__data(double d)
{
    __type = _dt_number;
    __p = new double(d);
}

This creates an object, that will persist until you manually free the pointer with delete. Which you should do in the destructor of __data:

__data::~__data()
{
    delete __p;
}

Update: I highly advise you don't use char * as your string type, but the C++ equivalent std::string. Also you may have a look at the library boost::any, which basically does the same thing as you want to achieve, but using templates instead.

Constantinius
  • 34,183
  • 8
  • 77
  • 85
  • Don't forget to add a delete in the destructor and when popped. – RedX Jul 16 '12 at 13:04
  • Thank you! I figured it would be an easy solution xD – zeluisping Jul 16 '12 at 13:07
  • The problem is whenever I implement a destructor my applications crash. – zeluisping Jul 16 '12 at 13:15
  • I don't think that the constructor is the problem. Why is your application crashing? Did you try using a debugger? – Constantinius Jul 16 '12 at 13:18
  • I'm using VS2010. Whenever I try define a destructor the application crashes with a "Debug Assertion Failed!" in the file `dbgdel.cpp` line 52. – zeluisping Jul 16 '12 at 13:25
  • That probably means that you are trying to `delete` something that should not be deleted. E.g: you cannot delete a function pointer, as it is not dynamically allocated. Specifically, you can only `delete` what you previously allocated with `new`, that you have to consider! – Constantinius Jul 16 '12 at 13:30
  • Using an operand of type `void*` to `delete` is invalid, arguably semantically invalid. – CB Bailey Jul 16 '12 at 13:57
  • The problem isn't with deleting, the problem is just by having a destructor declared the app crashes. – zeluisping Aug 06 '12 at 19:21
1
__data::__data(int i) 
{
    __type = _dt_integer;
    __p = &i; 
}
__data::__data(double d) 
{
    __type = _dt_number;
    __p = &d; 
}

because you get a pointer to a local variable

Constantinius
  • 34,183
  • 8
  • 77
  • 85
Ilya Lavrenov
  • 347
  • 1
  • 2
  • 7
1
__data::__data(int i)
{
    __type = _dt_integer;
    __p = &i;
}

You can't do that! The variable i doesn't exist after the function returns, so any pointer to it will contain garbage. You have to store the value of i in your __data class, not its address. You have (at least) two ways to go: use polymorphism (define a subclass of __data for each data type), or declare a union inside __data that contains a member for each data type that you support.

TonyK
  • 16,761
  • 4
  • 37
  • 72
0

The variables i and d are valid only during the life of the functions __data(int i), __data(double d). I mean those variable are created into the current stack and deleted when exiting the functions so you store a pointer to variable that won't be valid during the whole time your program is being run. When calling the Get function you access a pointer which points to a "dead" variable.

you should add these member to your class.

class __data
{
private:
  int iInt;
  double dDouble;
}

and modify your functions...to store the values and not the addresses...

__data::__data(int i)
{
    __type = _dt_integer;
    iInt = i;
}
__data::__data(double d)
{
    __type = _dt_number;
    dDouble = d;
}

Don't forget to modify the Get function.

void* __data::Get()
{
  switch( __type )
  {
    case _dt_integer:
      return (void*)iInt;
      break;
    case _dt_number:
      return (void*)dDouble;
      break;
  }

  return (void*)0;
}

then you can use it as below.

cout << "Int value " << (int)d.Get();
cout << "double value " << (double)d.Get();

It's a way to do what you want but the only one ! You can use a union member to store the values, to save memory usage.

A.G.
  • 1,279
  • 1
  • 11
  • 28
  • This potentially wastes a lot of memory, as many member variables are potentially never used... Also, what is the use of the `switch`? All you return is a `void*` so why bother casting it beforehand? – Constantinius Jul 16 '12 at 13:11
  • sure you can use a union to store the value...casting prevent compiler warnings !!! – A.G. Jul 16 '12 at 13:17
  • I see, then you should write this in your answer. You should not *cast, to prevent compiler warnings* because they are telling you that there is something wrong with your code. And an explicit cast just ignores this error. In your case, you explicitly cast an `int` or `double` to a pointer value, which is plain rubbish. The compiler tried to warn you, but got silenced. – Constantinius Jul 16 '12 at 13:21
  • I agree but sometimes it's really practical to do this kind of trick...who never did this ? – A.G. Jul 16 '12 at 13:49