AW: AW: [Clam-devel] [PATCH] Enum in DEBUG-mode
David García Garzón
dgarcia at iua.upf.edu
Fri Aug 3 07:46:13 PDT 2007
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);
> > >>>
> > >>> ________
> > >>
> > >> _______________________________________________
> > >> 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
>
>
> _______________________________________________
> 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