[Clam-devel] [PATCH] (work in progress) Harmonizer

[Hernán Ordiales]

On 7/9/07, Pau Arumi <parumi at iua.upf.edu> wrote:
[snip]
> Nice! Commited on revision 10365.
> However, I'd appreciate if you can send these 2 changes in a new patch
>
> 1) In CLAM we use "early returns" whenever possible, which
> simplifies the conditionals and prevent having nested blocks
[snip]
> So it'd look like:
>
> if (mHasDefault) return mDefault;
> if (!mBounded) return 0;
> return (mUpperBound+mLowerBound)/2;

ok, done (included in this patch and also written down in my mind as
clam style and indeed maybe adopted as own style too :P)

> 2) Control api is something very "core" and so it needs to be
> covered by unit tests. So please, try adding simple tests that
> checks the behaviour of SetDefault() in
> CLAM/test/UnitTests/ControlsTests/ControlsTest.cxx
> Actually unit tests not only assess that code is working, but also
> is a means for doing api specification.

ok, added

void testInControl_doesntHaveDefaultValueBydefault()
	{
		CLAM::InControl inControl("inControl");
		CPPUNIT_ASSERT_EQUAL( false, inControl.HasDefaultValue() );
	}
	void testInControl_defaultValue()
	{
		CLAM::InControl inControl("inControl");
		CPPUNIT_ASSERT_EQUAL( 0.0f, inControl.DefaultValue() );
	}
	void testInControl_settingDefaultValue()
	{
		CLAM::InControl inControl("inControl");
		inControl.SetDefaultValue(5.f);
		CPPUNIT_ASSERT_EQUAL( 5.f, inControl.DefaultValue() );
	}
	void testInControl_hasDefaultWhenTrue()
	{
		CLAM::InControl inControl("inControl");
		inControl.SetDefaultValue(0.f);
		CPPUNIT_ASSERT_EQUAL( true, inControl.HasDefaultValue() );
	}

> While we are on it, you could also doxygen document the
> DefaultValue() method which is not totally obvious. Do you know
> whow to do it? hint: /** doc */

yep ( i added doxygen comments previously in others patchs ;-) )
added too

> 3) DefaultValue() vs SetDefault()
> Probably I suggested the SetDefault name but now seeing the getter
> and setter names I feel that both should end with "Value"
> (SetDefaultValue). And maybe we should also rethink the getter name
> but not sure about this.

ok, fixed

> > hi, a new patch:
> > * Harmonizer prototype
> > * Fixed the issue about "copy connection name" of bounded inputs (for
> > compatibility with prototyper)
> Hi Hernan,
> could it be that this patch contains the last patch?

yes

> I'm saying
> because I got rejects on most of the files while patching. If so,
> please prepare a new one against last revision.

ok, attached in this mail, also with the changes above mentioned.
(in fact two separated patchs)

> Patches should be always independent (and mono-thematics ideally)
> when patches have dependencies lets use numbers. Example:
> 2-new-prototype.patch depends on 1-in-control-set-default.patch

ok, but...
for example, i know how to make patchs only from some files...(then i
can make them mono-thematics if i need) but, if i've updated one file
again from my first patch that wasn't uploaded to svn yet (in this
case, for example harmonizer)... how can i diff against svn and my
firsts patchs?

or i have to warn about discard first patch and only apply last? (but
in this case i think most cases patchs lose mono-thematic feature...)

cheers,
-- 
Hernán
http://h.ordia.com.ar
GnuPG: 0xEE8A3FE9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: harmonizer-net-prototype.patch
Type: text/x-diff
Size: 12102 bytes
Desc: not available
URL: <http://lists.clam-project.org/pipermail/clam-devel-clam-project.org/attachments/20070709/bffda726/attachment-0010.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: defaultfix-unittests.patch
Type: text/x-diff
Size: 9203 bytes
Desc: not available
URL: <http://lists.clam-project.org/pipermail/clam-devel-clam-project.org/attachments/20070709/bffda726/attachment-0011.patch>