Rockbox

I think the mailing list result was pretty clear. Just go for it.

Minor nitpicking: REPLAYGAIN_OFF could be == 0 as it makes slightly more sense, but that doesn’t actually change anything.

But adding the new option in the beginning (rather than at the end) would break the existing configurations (if somebody has set a value different from the default). Besides, I’d like that somebody take a look at the patch, I might have screwed something (but hopefully not).

I don’t think it breaks existing configs, but as I said, it doesn’t matter.

Looking at the patch, it looks fine.

I don’t see why REPLAYGAIN_OFF needs to be or even should at any specific position – it’s an enum, and the actual value of the enum doesn’t matter as you (at least should) only use the symbolic name anyway. It doesn’t change anything for existing configurations (i.e. cfg files) either as cfg files save the string representation, not any numerical value. Thus the enum value is simply an internal representation, so it’s perfectly safe to change that any time.

There’s a slightly different thing that caught my attention (but isn’t an issue of / should addressed with this patch): why is the type enum unnamed and the replaygain_type in the settings struct declared as int? Using a named enum instead would allow the compiler to warn if you use a value outside of the enum. Or did I miss any reason for using an int here?

@Dominik: having OFF=0 would allow to write something like “if (!replaygain_type)”. But I agree that this specifical case should be tested for by a dedicated comparison (which is done in the patch).

As to the usage of the enum: IIUC, it’s used just to defined some int values. Some #define’s could have been used as well. And I believe to have seen some code that uses a negative values as an indicator of some special condition which is then translated to some other value.