AW: AW: [Clam-devel] [PATCH] Enum in DEBUG-mode
Pau Arumi
parumi at iua.upf.edu
Fri Aug 3 08:24:40 PDT 2007
David,
I've also came up with the same solution in parallel with you.
However I didn't commit it because I was puzzled how I could not
make the bug to appear now that it's obvious.
Bernhard,
The last explanation really sheds the light where the real problem
is : the extensionMap struct in EAudioFileFormat. We want the
format as C enums (ints) there, as David have fixed.
static struct {
const char * extension;
EAudioFileFormat format;
} extensionMap[] =
{...}
However, after understanding the problem I've tried to reproduce it
and I was unable. After some debugging realized that the
CLAM_BEGIN_DEBUG_CHECK block is not executed being in debug mode
(scons release=0).
So this is another important bug to be fixed asap (on Monday
probably)
So great! many thanks for your reports and patience!
pau
En/na David García Garzón ha escrit:
> Hard to find out but i finally understood the problem. Just for the
> implementation of FormatFromFileName we are defining the second member of the
> struct as EAudioFileFormat and all the ints are created as Enums including
> the 0 terminator.
>
> The simple fix is to change EAudioFileFormat by a CLAM::Enum::tValue.
>
> Thanks for the report and your patience explaining us the problem.
>
>
> El Friday 03 August 2007 13:37:43 Rettenbacher, Bernhard va escriure:
>> Hi Pau,
>>
>> It seems to be a hard way to explain. Maybe the best thing is to broaden
>> the scope and show you what causes the failure:
>>
>> Assume a simple CLAM application reading an audio file do some descriptor
>> processing. It takes a file name as input and prints the descriptors to the
>> command line. One way to do this is creating a Network and using the
>> FreeWheelingNetworkPlayer to run it. Inside the player, I have a
>> MonoAudioFileReader: The reader gets a file name (e.g. with a "wav"
>> extension) and therefor has to call EAudioFileFormat::FormatFromFileName.
>> FormatFromFileName once creates the extensionMap, which has to construct
>> the array of structs containing EAudioFileFormat's. When the extensionMap
>> is filled, 20 string-value pairs have to be added to the extensionMap
>> array. Every time a pair is going to be added an EAudioFileFormat is
>> instantiated. The constructor calls the parent constructor Enum (const
>> tEnumValue * values, const tValue & value) which calls SetValue. After 19
>> string-value pairs containing a non-null string have been added to array, a
>> {0,0} ({NULL, 0} would be more readable) pair is added. SetValue cannot
>> find this {0,0}, so it asserts. But that would mean, it is not allowed to
>> add {0,0} to the extensionMap. Removing the {0,0} 'terminator' (and
>> changing the following for-loop stop criterion) was the solution, I sent
>> you in my first patch. David told me that you want to be consistent in
>> using zero-termination of arrays - resulting in the patch we're now
>> discussing. If you want to terminate the extensionMap in
>> FormatFromFileName, then you have to allow {0,0}. That is, what I think the
>> "expected behavior".
>>
>> I hope the lights are on now
>>
>>
>> Bernhard
>>
>> p.s.: how often do you make and execute debug builds of the library?
>>
>>> -----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: Donnerstag, 02. August 2007 12:31
>>> 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,
>>> 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);
>>>>>>
>>>>>> ________
More information about the clam-devel
mailing list