I committed v2-{0003,0004,0008,0009}, and I looked closer at the signed
versus unsigned mismatches and determined the following:* v2-0001: We are changing a variable from signed to unsigned, but the code goes out of its way to avoid negative values and signed integer overflow, so I don't think there are any real problems here. The only atomic arithmetic operation is in SICleanupQueue() where we subtract MSGNUMWRAPAROUND, which IIUC should never produce a negative value. That being said, I don't think it would be too disruptive to switch all relevant variables to uint32 as a prerequisite patch. I don't see any particular reason for those variables to be signed, anyway. * v2-0002: The variable in question stores a value from the SharedBitmapState enum. There's no atomic arithmetic involved: we just write and compare-exchange. At a glance, I didn't see any existing examples of using enum values for an atomic variable, but I think it's fine. I believe the C standard guarantees the enum values will be 0, 1, 2, etc., and even if we did set some enumeration constants to negative values, it wouldn't matter because we aren't doing arithmetic with it (and are probably unlikely to anytime soon). So, IMHO this one is fine as-is. * v2-0005: Since 0004 is committed, startupBufferPinWaitBuf is now a Buffer. Buffer is still a signed integer, but since we don't set startupBufferPinWaitBuf to a local buffer (only to a shared buffer or InvalidBuffer (0)), it'll always be >= 0. Furthermore, we don't do any sort of atomic arithmetic with this variable; it's hidden behind setter and getter functions. I think this one is fine. * v2-0006: The variables in this one are only ever incremented by 1, and they track the number of workers for a given operation, which I can't imagine approaches anything even close to overflowing an integer. Not to mention that we're using signed integers for all the relevant variables today... I don't see any risk here, but I'll try to switch the relevant variables to unsigned as a prerequisite and see how it looks. If it's too invasive, it's probably not worth worrying about. * v2-0007: I think this one already does all the work to avoid any signed versus unsigned mismatches. The Assert() in SharedFileSetOnDetach() looks bogus, though, so I'll fix that. I guess there could be some risk of overflow in the "refcnt + 1" in SharedFileSetAttach(), but we don't handle that at all today, so I don't think we need to worry about it. (In theory this patch actually reduces the overflow risk by switching to unsigned, anyway.) -- nathan
