Ptrdiff_t is evil

re: Ptrdiff_t is evil

gvisser — Fri, 06 Aug 2010 23:37:23 GMT

A better comparison is to test the sum vs the max:

if ( (m_pCurrent + cElements) > m_pMax ) throw;

[dcl] Except that any pointer addition outside of the buffer isn't defined by the standard, and could be optimized out, which is exactly what gcc did a while back. Regardless of what the compiler does, we shouldn't write non-standard code.

Your issue with

If ( ptrdiff_t < unsigned int )

becoming

If (__int64 < unsigned int )

is the result of Microsoft's decision to use the LLP64 Data Model, not because ptrdiff_t is evil.

[dcl] That's incorrect. It's the result of the C standard and how operator casts work.

re: Ptrdiff_t is evil

aaron — Fri, 16 Jul 2010 06:53:34 GMT

Which is why ptrdiff_t and size_t should always go hand in hand. The error is that cElements should be of type size_t.

[dcl] That and overloading the bounds check for the pointer difference to rule out negative value by using a 'clever' side-effect.

Also, I thought that, technically, it's possible for sizeof(ptrdiff_t) != sizeof(void*), but rather you're only guaranteed that sizeof(intptr_t) == sizeof(void*). In practice, though, you'll probably get by with either.

[dcl] I think you're right, but as you point out, in practice they're the same.

re: Ptrdiff_t is evil

david_leblanc — Fri, 26 Jun 2009 02:22:25 GMT

Someone wrote me, and pointed out:

You give the following example:

void IncPtr( unsigned int cElements )

{

if( m_pMax - m_pCurrent < cElements )

m_pCurrent += cElements;

else

throw;

}

The logic of this conditional statement seems messed up. Suppose the variables have been initialized like this:

static int array[10];

m_pCurrent = array + 5;

m_pMax = array + 10;

i.e. m_pMax points to the end of the array and m_pCurrent points somewhere in the middle. From the names, I expect this is how the variables would be used.

Then, something calls IncPtr(2). Now, m_pMax - m_pCurrent is 5, and 5 < 2 is false, so the function throws (assuming it's called from a catch clause) as if there had been an error. However, it would have been perfectly safe to increment m_pCurrent by 2.

On the other hand, if something calls IncPtr(99), then 5 < 99 is true, and the function happily does m_pCurrent += cElements, incrementing the pointer far past the end of the array.

Thus, the test should be inverted. And if you do that, then the behaviour in 64-bit builds, where the unsigned int gets converted to ptrdiff_t without any compiler warnings, will actually become correct; and the 32-bit builds that trigger a warning will exhibit the bug.

[dcl] This has now been fixed. The overall point that the branch is different depending on build is the important bit.

re: Ptrdiff_t is evil

Shane Hatagawa — Sun, 05 Apr 2009 23:57:52 GMT

Is taking _either_ branch of this function a good thing? You imply that having the function throw is 'safe', but the class is not in a valid state, right? Any attempt to use the class is already going to access an out of bounds element, changing m_pCurrent from one invalid value to another isn't going to help, but throwing from this function isn't going to help either. Ideally this class would be checking invariants, and would have caught this when the error occurred. As for the secure choice in this circumstance? I don't see how you could do anything safe here except terminate (if possible saving as much user state for a better restart/recover). None of this takes away from your discussion here, but sometimes in the face of broken program state, there is no safe alternative except to just stop the train.

[dcl] good points, but it is just an example, and what's the right error path is very dependent on a lot of context we don't have. The main point is that give the same input, we go down different branches on 32 and 64-bit, which is a really nasty trick.

a-foton » Ptrdiff_t is evil

a-foton » Ptrdiff_t is evil — Wed, 03 Sep 2008 02:20:26 GMT

PingBack from http://blog.a-foton.ru/2008/09/ptrdiff_t-is-evil/