What's the point in Max(Threading.Interlocked.Increment(Offset), Offset - 1)?

Question

I have seen the mentioned piece of code on several occasions now, what's the point of doing a Max(a+1, a-1)? At first I thought it might be to prevent underflow, but it doesn't really make sense not to prevent the underflow in that case.

Answer 1

A bit of googling gives me a suspicion that this might arise from some (possibly buggy) C# to VB.NET convertor software. That would explain the frequent appearance of it.

Added: Yessss, I found it! Try entering the following C# code in it and convert to VB.NET:

int i;
i++;

Answer 2

As you mentioned, it's possible they are trying to detect when Offset wraps to zero (or goes negative) and keep it pegged at the max value for whatever type Offset is. Or they may be trying to deal with a situation where Offset is 'simultaneously' incremented in multiple threads (or something).

In either case, this is buggy code.

If they are trying to detect/prevent wrap to zero or negative, one too many increments will cause the problem anyway. If they are trying to deal with Offset being incremented 'simultaneously', I'm not sure what problem they're really trying to solve or how they are trying to solve it.

Here are the problems:

As Jon Skeet says:

However, it still sounds odd to me - because unless Offset is volatile, the latter expression may not get the latest value anyway.

But it's even worse than that - even if Offset is volatile there's nothing to serialize reading the new value of Offset with another thread that may be incrementing it, so the very instant after the Max() expression re-reads the value of Offset any number of other threads may come along and increment it (any number of times). So at best, the expression is useless, but using that technique can be harmful because you may end up using a value of Offset that doesn't 'belong' to you.

Consider the situation where you're using Offset to track an array index or something (which given the name sounds like exactly what might be going on). When you atomically increment Offset the array element at that index becomes yours to use. If you use the Max() expression as in the question, you may suddenly be trampling on an array element that really belongs to another thread.

I can't think of a legitimate reason to use

Max(Threading.Interlocked.Increment(Offset), Offset - 1)

Like I said, at best it's harmless (and useless), but it could introduce very hard to diagnose problems - don't use it.

---

Thanks to the comment by Simon Svensson that pointed to a Google search for the usage in the question, it looks like this code usually (always?) comes from the output of a C# to VB.NET converter. It even uses this expression to increment local variables (so threading isn't an issue). In the cases where Offset is a local variable, this expression falls into the useless but mostly harmless variety (there will be a 'bug' if the increment wraps). It's really more or less a very inefficient Offset++ expression (in fact it appears to be the way the converter converts the C# expression Offset++ to VB.NET - see http://www.telerik.com/community/forums/open-source-projects/code-converter/added-value.aspx#307755). Actually this conversion is buggy for Offset++ anyway because it returns the incremented value when it should return what the value was before incrementing.

The really bad thing is that other people may look at that code and think, "so that's the way I need to use Interlocked.Increment()".