Refactoring to support maps in the attribute system. Fixes bug 1352

Refactoring to support maps in the attribute system. Fixes  https://www.nsnam.org/bugzilla/show_bug.cgi?id=1352

[Tom Henderson, 2012-04-18 13:28:46]

Thank you Jaume; just one question I have in the test code.

http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-sui...
File src/core/test/attribute-test-suite.cc (right):

http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-sui...
src/core/test/attribute-test-suite.cc:138: MakeObjectVectorAccessor
(&AttributeObjectTest::DoGetMapN,
Is TestMap2 ever tested?  I would like to see sample code of the setter actually
working through a method named "DoGetMapN()"

Likewise for TestVector2 above (which appears to be untested).

http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-sui...
src/core/test/attribute-test-suite.cc:765: NS_TEST_ASSERT_MSG_EQ (map.GetN (),
0, "Initial count of ObjectVectorValue \"TestMap1\" should still be zero");
I find this test code to be counter-intuitive.  I realize it is copy/replaced
from another test, but can this class support the typical SetAttribute() and
SetAttributeFailSafe() operations?  The AddToMap1() seems to be just a custom
test class method and doesn't represent real usage.

[Jaume Nin, 2012-04-20 13:08:39]

Hi Tom,

You are absolutely right, I did not like much the testing being performed
either and accessors for map and vector were not tested. I did a quick test
and it failed so I'll keep it on the background and during next week try to
provide a better test case and fix if needed.

I focused on the direct access since it is the one widely used in LENA so
even if it is not perfect it still should work.

Enjoy your weekend,
Jaume

On Wed, Apr 18, 2012 at 3:28 PM, <tomh.org@gmail.com> wrote:

> Thank you Jaume; just one question I have in the test code.
>
>
> http://codereview.appspot.com/**6047044/diff/1/src/core/test/**
>
attribute-test-suite.cc<http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-suite.cc>
> File src/core/test/attribute-test-**suite.cc (right):
>
> http://codereview.appspot.com/**6047044/diff/1/src/core/test/**
>
attribute-test-suite.cc#**newcode138<http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-suite.cc#newcode138>
> src/core/test/attribute-test-**suite.cc:138: MakeObjectVectorAccessor
> (&AttributeObjectTest::**DoGetMapN,
> Is TestMap2 ever tested?  I would like to see sample code of the setter
> actually working through a method named "DoGetMapN()"
>
> Likewise for TestVector2 above (which appears to be untested).
>
> http://codereview.appspot.com/**6047044/diff/1/src/core/test/**
>
attribute-test-suite.cc#**newcode765<http://codereview.appspot.com/6047044/diff/1/src/core/test/attribute-test-suite.cc#newcode765>
> src/core/test/attribute-test-**suite.cc:765: NS_TEST_ASSERT_MSG_EQ
> (map.GetN (), 0, "Initial count of ObjectVectorValue \"TestMap1\" should
> still be zero");
> I find this test code to be counter-intuitive.  I realize it is
> copy/replaced from another test, but can this class support the typical
> SetAttribute() and SetAttributeFailSafe() operations?  The AddToMap1()
> seems to be just a custom test class method and doesn't represent real
> usage.
>
>
http://codereview.appspot.com/**6047044/<http://codereview.appspot.com/6047044/>
>

[Tom Henderson, 2012-05-08 05:47:47]

On 2012/04/20 13:08:39, Jaume Nin wrote:
> Hi Tom,
> 
> You are absolutely right, I did not like much the testing being performed
> either and accessors for map and vector were not tested. I did a quick test
> and it failed so I'll keep it on the background and during next week try to
> provide a better test case and fix if needed.
> 
> I focused on the direct access since it is the one widely used in LENA so
> even if it is not perfect it still should work.

In looking at the code again tonight, I'm not sure that it is intended to be
able to set these through SetAttribute.  I am still not clear on why DoGetMapN()
is provided as a setter, however.

I suggest the following to proceed:
- delete TestMap2 test attribute (unused and not understood)
- keep bug 1352 open for handling strings and also for resolving the question on
attribute tests
- otherwise merge your patch so LENA merge can proceed

[Mathieu Lacage, 2012-05-19 07:09:35]

On 2012/05/08 05:47:47, Tom Henderson wrote:

> In looking at the code again tonight, I'm not sure that it is intended to be
> able to set these through SetAttribute.  

These are not supposed to be settable through the attribute system.


> I am still not clear on why DoGetMapN()
> is provided as a setter, however.

I do not understand that

> 
> I suggest the following to proceed:
> - delete TestMap2 test attribute (unused and not understood)
> - keep bug 1352 open for handling strings and also for resolving the question
on
> attribute tests
> - otherwise merge your patch so LENA merge can proceed

+1

thank you for pushing this forward