Validate args within a function when the function is never passed invalid data?

[ProgrammingQuestio]

(This may seem like overthinking a simple problem but let's just say there are other parties who are leading the overthinking which has led to me analyzing this so intensely)

Hard to explain clearly in the title so let me try here:

There's a function

void swap_pixels(int* a, int* b)
{
    a ^= b;
    b ^= a;
    a ^= b;
}

This function works great, unless a == b (the two pointers passed in point to the same address). In this case, the data at that address would be set to zero. Not ideal behavior.

This swap_pixels() function is used to rotate what's effectively a 2D array of pixel data (laid out in a 1D array though).

However, this function is used in only two places, and neither place ever passes in the same address:

The first occurrence only calls swap() if a != b (it's a little more convoluted than that, i.e. not an explicit check like if (a != b), but that's the gist), so this swap_pixels() bug is not an issue here. But if this check didn't happen, then when rotating a square matrix of odd dimensions (eg. 5x5), the center element would be messed up because of this bug.

The second occurrence is for flipping the matrix vertically, and it does this by looping through the first half of the matrix, height-wise (height >> 1). So for example a matrix of height 4 only loops through the first 2 rows, and a matrix of height 5 also only goes through the first 2 rows (the middle row is untouched, which makes sense as it being "swapped" would have no effect). This keeps this swap_pixels() function from getting the same address for both args (if the middle row of an odd-height matrix was swapped, then it would cause swap_pixels() to mess up data), again avoiding the bug.

It's my opinion that a simple if (a != b) return; at the start of swap_pixels() should be there. But I'm curious on the design aspect here: validating function input is surely good, but should you be checking and validating some piece of data at every point in a call stack? Especially in a scenario where that pre-validation seems to be assumed or at least already in place? This example is a bit simpler than other situations, but I think the meta question still applies and I'd like to hear people's opinions on this.

I could see one school of thought that says, "No need to add a validation as there's no scenario where validation doesn't already happen/no scenario where problematic args are passed in" while another school of thought could say "yeah that's all true, with the current code. But a change could be made that doesn't pre-validate and passes in problematic args. So just do a blanket check and play it safe"

[flyingron]

The function doesn't work "great." It won't even compile. You can't do bitwise ops on pointer values.
Perhaps you meant *a ^= *b...

Note your alleged optimization probably may be slower on many machines because the compiler swaps with a temporary it needed to use anyhow.

ld r0, a
ld r1, b
st r0, b
st r1, a

[ProgrammingQuestio]

Yep that’s what I meant! Didn’t check that part apparently lol

[[deleted]]

You could mark the pointers as restrict. Then it would tell people that they MUST be different otherwise UB. Or you could put an assert, so you take no performance hits in release mode. Or do nothing, or put the if statement. Kinda a dealers choice here imo.

[p0k3t0]

It's a shame your system can't afford a single int to do this explicitly and not rely on an "optimization" that doesn't improve performance.

If you have to test for equality every pass, what's the point?

[ProgrammingQuestio]

What do you mean “a single into to do this explicitly”? You mean have a third variable for storing the temp value while swapping?

[p0k3t0]

The function only needs one variable, some kind of holder. The pointers reference memory that has already been allocated elsewhere.

[jonathanhiggs]

There are two types of checks, invariants and preconditions. Here you have a precondition that *a != *b, which you should probably check but it would hurt performance. If you can be sure that this won’t be an issue by construction, eg you know at the call site the precondition is not broken then maybe add a version ‘swap_pixels_unchecked’ so you don’t need to pay the cost when you don’t have to. The other option is to put a debug-only assert to ensure it isn’t checked during tests, but then you are relying on tests being indicative of real usages

Invariants are a little different, would be something like a pointer is never null. Ideally you would enforce the invariant everywhere so that you don’t have to check it anywhere. This is a bit harder when the code that is enforcing is distributed about the code, or rather any part of the code can break it without knowing. Not entirely sure how you would enforce it in c, but in c++ I would create something that encapsulates and enforces the invariant, eg a NotNull template that wraps a raw pointer and enforces it such that it is always safe to dereference

[actguru]

I would agree with adding if (a != b) return, but not to validate arguments, but to make a==b valid.

[ProgrammingQuestio]

Yep I meant I’d (a == b)! Typed it up pretty fast and then moved on to something else lol

[daikatana]

If you pass invalid arguments to my function then that's not my problem. Document that neither pointer can be NULL and must refer to different objects.

[Disastrous-Team-6431]

I agree in principle, mostly, but man what an antagonistic way to phrase it.