AW: AW: [Clam-devel] [PATCH] Enum in DEBUG-mode
Rettenbacher, Bernhard
bernhard.rettenbacher at joanneum.at
Fri Aug 3 04:37:43 PDT 2007
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);
> >>>
> >>> ________
> >> _______________________________________________
> >> 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-de
> > vel
>
>
> _______________________________________________
> 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