AW: [Clam-devel] [PATCH] Enum in DEBUG-mode
Pau Arumi
parumi at iua.upf.edu
Thu Aug 2 03:30:52 PDT 2007
En/na Rettenbacher, Bernhard ha escrit:
> Hi Pau,
Hi Bernhard,
I feel we do not agree on what the "expected behaviour" is, but I
hope are close to put light upon this problem...
> Ok, maybe the best way to show you "the obvious", is to give you a
> detailled explanation (with code):
> In EAudioFileFormat::FormatFromFilename, an array of extension
> enumerators is created.
>
> <code>
> ...
>
> static struct {
> const char * extension;
> EAudioFileFormat format;
> } extensionMap[] =
> {
> {"wav", eWAV},
> {"ogg", eVorbisMk1},
> {"aif", eAIFF},
> {"aiff", eAIFF},
> {"au", eAU},
> {"snd", eAU},
> {"raw", eRAW},
> {"paf", ePAF},
> {"svx", eSVX},
> {"nist", eNIST},
> {"voc", eVOC},
> {"ircam", eIRCAM},
> {"w64", eW64},
> {"mat4", eMAT4},
> {"mat5", eMAT5},
> {"mat", eMAT5},
> {"mp1", eMpegLayer1},
> {"mp2", eMpegLayer2},
> {"mp3", eMpegLayer3},
> {0,0}
> };
>
> ...
> </code>
> This subsequently instantiates Enum's. In the Enum constructor,
Actually this array is not used to instantiate the Enum
constructor. This is just a helper method to obtain the format
(enum type) from an extension.
The Enum constructor is initialized with EnumValues()
static tEnumValue * EnumValues()
{
static tEnumValue sEnumValues[] = {
{ eWAV, "WAV" },
{ eAIFF, "AIFF" },
...
{ 0, 0 }
}
Note that the string/enum columns are reversed --though I don't
think this was a source of confusion.
> SetValue
> is called. SetValue is also called for the {0,0} pair. The expected
I don't understand what do you mean by "SetValue is called for the
{0,0} pair.
You have these two constructors with parameters:
EAudioFileFormat( "WAV" ) and
EAudioFileFormat( EAudioFileFormats::eWAV )
Only the second one will call void SetValue(const tValue v).
If you call SetValue(eWAV) which equals to SetValue(0) since eWAV
is the first element of the enum, then it will enter to the for
loop (because mEnumValues[0].name is not 0)
and will return on the first iteration (because
mEnumValues[0].value is 0)
> functionality is, that the for loop iterates to the last entry in the
> mEnumValues array and finds the {0,0} pair. But the loop exits at the
> time, when the last entry is reached and before v is compared to the
> entry. So, afterwards the CLAM_ASSERT(false,...) statement is executed:(
{0,0} means end-of-list. So when the last entry {0,0} is reached
(condition mEnumValues[i].name==0) it means that the value was not
found, so we must assert.
An example, (was also in my last mail), our expected behaviour for
SetValue(0) on this enum
{
{1, "WAV"},
{0,0}
}
is _assert_ because only 1 is a valid value.
I guess you want to consider 0 always a valid value. Is that right?
pau
> In Enum.hxx:
> <code>
> void SetValue(const tValue v) {
> CLAM_BEGIN_DEBUG_CHECK
> for (int i = 0; mEnumValues[i].name; i++)
> if (v==mEnumValues[i].value) {
> mValue = v;
> return;
> }
>
> CLAM_ASSERT(false, "Invalid value for an Enum");
> CLAM_END_DEBUG_CHECK
> mValue = v;
> }
> </code>
>
> What we did change: We made the loop to first compare the value with the
> entry and afterwards look, whether we have reached the end of the
> mEnumValues array. So, if we reach the last entry ({0,0}), the
> comparison takes place, we set the mValue and then return.
>
> <code>
> void SetValue(const tValue v) {
> CLAM_BEGIN_DEBUG_CHECK
> int i = 0;
> do {
> if (v==mEnumValues[i].value) {
> mValue = v;
> return;
> }
> } while (mEnumValues[i++].name);
> CLAM_ASSERT(false, "Invalid value for an Enum");
> CLAM_END_DEBUG_CHECK
> mValue = v;
> }
> </code>
>
> The only constraint is, that the mEnumValues array has to have at least
> one element. But this is also true for the original code. Maybe there
> should be (or is) some check elsewhere, whether the mEnumValues array is
> correctly terminated by {0,0}.
>
> Another issue: This may also concern other SetValue-like methods
> (SetValueSafely,...)
>
>
> Cheers,
>
> Bernhard
>
>> -----Ursprüngliche Nachricht-----
>> Von: clam-devel-bounces at llistes.projectes.lafarga.org
>> [mailto:clam-devel-bounces at llistes.projectes.lafarga.org] Im
>> Auftrag von Pau Arumi
>> Gesendet: Mittwoch, 01. August 2007 11:18
>> An: clam-devel at llistes.projectes.lafarga.org
>> Betreff: Re: AW: [Clam-devel] [PATCH] Enum in DEBUG-mode
>>
>> 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);
>>>
>>> ________
>> _______________________________________________
>> Clam-devel mailing list
>> Clam-devel at llistes.projectes.lafarga.org
>> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo
> /clam-devel
>
> _______________________________________________
> Clam-devel mailing list
> Clam-devel at llistes.projectes.lafarga.org
> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel
More information about the clam-devel
mailing list