AW: [Clam-devel] [PATCH] Enum in DEBUG-mode

[Pau Arumi]

En/na Rettenbacher, Bernhard ha escrit:
> Hi Pau,
>  
> I have a little error in the description:  The wrong assertion happens, when the string and the value are 0. The return statement in the for-loop will never be reached, so the program continues to the assertion, which will always fail.
>  
> The error is quite hard to see, but if you build in debug mode, there's no way to overlook it.

Hi Bernhard,
maybe I don't see the obvious. Could you specify which enum 
content (i.e. {{"wav",  0}, {0,0}} ) and SetValue parameter 
produce the undesired effect?

pau



> 
> En/na Rettenbacher, Bernhard ha escrit:
>> Dear CLAM-Developers,
>>
>> About 1 or 2 months ago, I sent you some patches, one of them was for EAudioFileFormat::FormatFromFilename. You rejected it, because I did break your rules how to handle arrays of structures. Now - as I made a build of the current sources - we had an eye again on this problem:
>>
>> When calling FormatFromFilename, an array of Enums, containing the possible file extensions, is created. This array is terminated by a {0,0}-pair. In DEBUG-mode, this causes an assertion in Enum::SetValue, because a null string is the stop criterion in the for-loop. So, if the value, you want to set is 0, the code after the for-loop is executed, which is CLAM_ASSERT(false,...). In RELEASE mode, this loop and the assertion are never executed.
>>
>> Our solution replaces the for loop in Enum::SetValue by a do-while loop, which takes the {0,0}-pair in account.
> 
> I think the code have the intended behaviour. But maybe I don't
> understand your claim.
> 
> Enum::SetValue(v) stores v into a member of the enum.
> 
> In debug we want to check (assert) that v is one of the valid
> options of the enum. In release we don't: just assign v to the
> member.
> 
> You say: if the value you want to set is zero (SetValue(0)) it will
> assert.
> If the enum is defined with 0 as a possible value then it should
> not assert. For example
> {
>         {"wav",  0},
>         {0,0}
> }
> Since the iteration is finished when mEnumValues[i].name==0 not
> .value==0}
> 
> 
> On the other hand the patch version have a problem. Take this enum:
> {
>         {"wav",  1},
>         {0,0}
> }
> SetValue(0) will not assert when it should since 0 is not an
> accepted value.
> 
> Cheers
> Pau
> 
> -----
> 
> Current code:
> 
> for (int i = 0; mEnumValues[i].name; i++)
>         if (v==mEnumValues[i].value) {
>                 mValue = v;
>                 return;
>         }
> 
> 
> Patch:
> 
> int i = 0;
> do {
>         if (v==mEnumValues[i].value) {
>                 mValue = v;
>                 return;
>         }
> } while (mEnumValues[i++].name);
> 
> ________