AW: AW: [Clam-devel] [PATCH] Enum in DEBUG-mode

[Rettenbacher, Bernhard]

Hi Pau, 

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, SetValue is called. SetValue is also called for the {0,0} pair. The expected 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:(

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
>