Proper use of casting

[thomasloven]

Hi.

A year or two ago, I posted the following code on my website:

Code: Select all

void pmm_free_page(unsigned int p)
{
	// Sanity check
	p &= PAGE_MASK;
	
	// Throw away irrecoverable pages
	// (will also catch null pointers)
	if(p < pmm_pos)
		return;	
	
	if(pmm_page_stack_top >= pmm_page_stack_roof)
	{
		// If there's no more space in the stack
		// use the freed page to expand it
		 vmm_page_set(pmm_page_stack_roof, VMM_PAGE_VAL(p, PAGE_PRESENT | PAGE_WRITE));
		pmm_page_stack_roof += PAGE_SIZE;
	} else {
		// Add the freed page to the top of the stack
		// and increase the stack pointer
		*((unsigned int*)pmm_page_stack_top) = (unsigned int )p;
		pmm_page_stack_top += sizeof(unsigned int *);
	}
	// When there's at least one page in the stack, the PMM is good to go
	pmm_running = TRUE;
}

And got the following comment

[...]
I haven't read this one but the first I found after pressing CTRL+F and searching for "*)" was exactly what I expected: improper use of casting.
[...]

Since then I have been trying to figure out in what way my use of casting is wrong and how to do it right.

Would it be better to use

Code: Select all

    uint32_t *stack = (uint32_t*)pmm_stack_loc;
    *stack = p;
    pmm_stack_loc += sizeof (uint32_t);

as in jamesm-tutorials?
If so, what's the difference and why?

Best regards
/ Thomas

[bluemoon]

For C, I don't see any problem with the old school casting, whether it's ugly or not is merely personal taste.
For C++, you may use reinterpret_cast to convert numbers to pointers (assume they are of same size).
PS. casting numbers to/from pointer of different size would most likely cause you trouble.

[Combuster]

improper use of casting.

What I see is a variable that holds an address for storing data, which is neither a pointer (textbook use), nor is obviously named as being a numerical memory address (arguably proper use in OS development context, in which case it should at least be an intptr_t).
The effect is pretty obvious here: using the proper types reduces the two rather verbose statements to its most concise and obvious form:

Code: Select all

*pmm_page_stack_top++ = p;

I also see an "unsigned int *" rather than void** / intptr_t* (or even uint32_t *) which is a typical instance of the antipattern "avoiding the use of the proper definitions in the C standard".

[thomasloven]

Thanks.
That's a good explanation.

For some reason, though, I never got that to compile before, even in my latest rewrite (where I am using fixed width integers, btw). I just tried again now, and it worked.

Dunno what I did wrong before, but this made for a lot prettier code. It also made me find a terrible bug waiting to happen...