6

I'm trying to create a struct that can hold generic values. The code below works but I'm getting a compiler warning about the cast from pointer to integer. This is on a 64-bit system.

struct node
{
    void *key;
    void *value;
};

void insert(struct node *ht, void *key, void *value)
{
    ht->key = key;
    ht->value = value;
    return;
}

int main()
{
    struct node *t = (struct node *)malloc(sizeof(struct node));
    insert(t, (void *)3, (void *)5);
    printf("[%d]->[%d]\n", (int)t->key,(int)t->value);
    free(t);
    return 0;
}

I'm not even sure if this is the correct way to go about it. I sort of hacked it up. Please let me know if there is a proper way to do this.

DavidL
  • 905
  • 3
  • 10
  • 14
  • 1
    Is there a reason you're not just using `int`s to begin with? – Mike Feb 05 '13 at 15:51
  • 1
    In C, please don't [cast the return value of `malloc()`](http://stackoverflow.com/a/605858/28169). – unwind Feb 05 '13 at 15:53
  • 3
    If you're going to use void pointers, use them to store pointers, not integers. (And there are arguments for and against casting the return from `malloc()`; you don't _have_ to avoid it, but you should know _why_ you do or do not do the casting.) – Jonathan Leffler Feb 05 '13 at 15:53
  • 1
    You may have 64-bit pointers, but you are probably still using 32-bit ints. (Or less likely, vice versa!) – metal Feb 05 '13 at 15:54
  • @Mike I want `node` to be able to hold any type not just `int`. – DavidL Feb 05 '13 at 15:58
  • 2
    WHY use pointers to store int data? – Davide Berra Feb 05 '13 at 15:58
  • 1
    But `void *` isn't necessarily big enough to hold `long double`. – Jonathan Leffler Feb 05 '13 at 15:58
  • @DavideBerra How would I go about it then ? Or do you mean that `void *` should point to `int *`, `char *` etc ? – DavidL Feb 05 '13 at 16:09
  • @JonathanLeffler - `But void * isn't necessarily big enough to hold long double` - I wasn't aware of a size limitation on a `void *`, could you specify what this is? – Mike Feb 05 '13 at 16:09
  • 1
    @Mike: confusion reigneth—on your side, I think. A `void *` has a fixed size (normally 32-bits on a 32-bit system, 64-bits on a 64-bit system). A `void *` can be used to store a pointer to data of arbitrary size (though you'll need to know how to access the data through the pointer to get the correct results). However, you're not storing pointers in your `void *`; you're storing `int` values. My point about `void *` not being big enough to hold a `long double` value is that on a 64-bit Mac, `sizeof(long double) == 16 && sizeof(void *) == 8`, for example. A `void *` can hold a `long double *`. – Jonathan Leffler Feb 05 '13 at 16:16

3 Answers3

9

The compiler tries to warn you that you lose bits when casting from void * to int. It doesn't know that the void * is actually an int cast, so the lost bits are meaningless.

A double cast would solve this (int)(uintptr_t)t->key.
It first casts void * to uintptr_t (same size, no warning), then uintptr_t to int (number to number, no warning).
You need to include <stdint.h> to have the uintptr_t type (an integral type with the same size as a pointer).

ugoren
  • 16,023
  • 3
  • 35
  • 65
  • 1
    `sizeof(size_t)` is not guaranteed to equal `sizeof(void *)` (what is the state of play on Windows 64?). If you want a reliable type that is the size of a pointer, use `uintptr_t` from `` (or ``). – Jonathan Leffler Feb 05 '13 at 15:56
  • 1
    @JonathanLeffler, I think `size_t` is OK for WIN64 (unlike `long`), but in principle you're right. I'll change. – ugoren Feb 05 '13 at 15:58
  • I wouldn't suggest a double cast. The first one wasn't necessarily necessary. The second one isn't going to be any better. – Josh Petitt Feb 05 '13 at 16:52
  • 1
    @JoshPetitt, For overcoming strict compiler warnings, the double cast may be necessary. The only alternative is to design the data structures differently, which is better, but not always applicable. – ugoren Feb 05 '13 at 19:38
  • @ugoren, I would choose the alternative :-) Casting is often used to "hush that pesky compiler!", instead of asking, "hmm, I wonder why the compiler is warning me?" For a new C user, I would tell them to never cast. If you are casting, you are probably doing it wrong. – Josh Petitt Feb 05 '13 at 20:14
  • 1
    @JoshPetitt, you're right that I offered the easy solution, not to fundamental one. But using `void *` to carry arbitrary types, including integers, is quite common and useful, yet leaves no better alternative. – ugoren Feb 05 '13 at 21:24
3

One thing that is useful to remember is that in a 64 bit system, a pointer is a 64 bit value [a memory address.]

int is only a 32-bit value, regardless of what architecture you're on. Anytime you try to assign a 64-bit value in to a 32-bit value without explicitly casting it the compiler will throw a warning [granted, it could still work but in general is not good practice.]

If you're not averse to using unsigned integers, it might be easier to use uint_64t or something like that, which will avoid the 64-bit to 32-bit assignment (uint_64t is an unsigned 64-bit int)

Hope that helps.

One thing you could do is:

int key = 3;
int value = 5;
insert(t, (void *) &key, (void *) &value);
printf("[%d]->[%d]\n", (int) *(t->key), (int) *(t->value));

However, be very very careful when doing things like this. It's impossible to absolutely know what the value being stored by that pointer is unless you can guarantee that you set the pointer and it's value/type is unchanged. Unless you add an enum field or something that stores the type of value stored at the pointer location-- but that kind of defeats the purpose.

HodorTheCoder
  • 254
  • 2
  • 11
  • the OP will also have to be careful not to do something like this in a function. The ints would be "on the stack" and invalid once the function returns. – Josh Petitt Feb 05 '13 at 17:59
2

You've got lots of problems with the code you posted.

First:

printf("[%d]->[%d]\n", (int)t->key,(int)t->value);

When printing pointers, print pointers. Use %p. See here:

http://www.cplusplus.com/reference/cstdio/printf/

Second:

insert(t, (void *)3, (void *)5);

I'm not sure what you expected to happen, but you are storing the ADDRESS not the VALUE. (i.e. you are setting your key as a pointer to the contents of address 3, and the value as a pointer to the contents of address 5).

Third:

struct node *t = (struct node *)malloc(sizeof(struct node));

Here you have allocated the node, but you have not allocated anything to hold the contents. Your node just holds some pointers (ADDRESSES), it doesn't hold any VALUES. I'm not sure if this is what you want.

Finally, your compiler is right in warning you. There is a chance you are losing precision. Like @JonathanLeffler said, use uintptr_t.

Josh Petitt
  • 9,371
  • 12
  • 56
  • 104