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