Bug 1352

patch is full of un-needed whitespace changes. Please, resubmit for review.

Created attachment 1356 [details]
Patch without the unneded whitespaces

Sorry for the long delay, the comment just slept through my inbox and I completely forgot.

Extra whitespaces where added by check-style.py, hope this one suits better.

Regards,
Jaume

I am afraid that:

  - the latest patch does not include the new files you created. I tried to extract them from the old patch but had to edit it by hand to do so: neither hg import nor patch understood it (how on earth did you generate that first patch ??)

  - the latest patch still has some whitespace stuff which makes reading this a painful exercise.
 
  - you have failed to provide any hints as to why you have made these changes and/or provide a high-level description of your changes.

I would suggest that before you post a new patch for review, you try to review the code yourself and put yourself in the shoes of someone who works on that code about 30mins each week (are you into role playing games ?): what can you do to make your reader happy and willing to go further ? I am sure it will make a huge difference.

Anyhow, I think I reverse engineered your original problem from the patch (I still have no clue as to what you use this map support for: followup patches will need to provide some background on this and if you did provide it in your first review request ages ago, I would suggest you think really hard about context, and the lack of it) and no, I am not happy with the need to support two kinds of containers to iterate the object tree. 

With the above disclaimers about my lack of understanding of the big picture, I would suggest you merge these two types of objects (the map & the vector) into one that supports a map-like API instead of a vector-like API. Namely, maybe just keep the ObjectPtrMap* classes and kill the other one by making sure that MakeObjectVectorAccessor and MakeObjectMapAccessor both create a single type of ObjectPtrMapValue that has the following API (basically, the one you used originally but tweaked to return the pair key/value:

class ObjectPtrMapValue 
{
public:
  std::pair<uint32_t, Ptr<Object> > Get (uint32_t i) const;
};

I have no idea what would be the impact of such a change on the existing Config/ConfigStore code. Before you embark on this, I would suggest you try to provide (again maybe, sorry) some background information on what you are trying to do at the model level.

Hi Mathieu,

First of all, apologies for the inconveniences and the little dedication I'd been able to put in this issue and the missing files on the last submitted. Patch generation is a brand new topic for me and it seems there is a lot of room for improvement here.

Back to the basics. The main motivation for this feature is that in LENA we have several map containers that need to be made available through the attribute system in order to access the traces. The containers need to be maps since their keys are not consecutive. A sample path of what we need follows:
/NodeList/#nodeId/DeviceList/#deviceId/LteUeRrc/RadioBearerMap/#RNTI/LteRlc/TxPDU

#RNTI is simply a user ID unique to the LTE network, hence we have things like the following:
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/2/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/3/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/4/LteRlc/TxPDU

The current implementation, translates the map to a vector, hence the keys are missed and the paths generated are:
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/0/LteRlc/TxPDU
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/0/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU

The original idea was to avoid touching already working stuff, hence the duplicity. However, from the maintainer point of view, it makes much more sense to have a single ObjectPtrMapValue for both vectors and maps and probably the solution will me more seamless, just the API and the container.

If you agree with this direction I'll try to provide the patch (clean and with just the right number of whitespaces)  and extend the testing to cover maps as well within a couple of weeks.

Regards,
Jaume

(In reply to comment #4)

> #RNTI is simply a user ID unique to the LTE network, hence we have things like
> the following:
> /NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
> /NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/2/LteRlc/TxPDU
> /NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/3/LteRlc/TxPDU
> /NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/4/LteRlc/TxPDU

how do you allocate the user ids ?

(In reply to comment #5)
> (In reply to comment #4)
> how do you allocate the user ids ?
We have two different IDs, radio IDs are sequentially allocated in the system (first UE id 1)  but on the eNodeB you can have a non sequentiall collection of radio IDs. Logical channel IDs are sequentially allocated within the eNodeB, but again, there is no direct mapping since for instance one UE can use more than one logical channel.

Mathieu and I discussed this issue while attending WNS3 2012, here is the summary:

1) the use case for having maps of objects is agreed upon, since there are several cases (in addition to the example posted by Jaume) where it is needed to provide to the user a list of objects with potentially non-consecutive IDs;

2) that considered, Mathieu suggested to refactor the patch so as to always use maps under the hood, in order to avoid most of the code duplication and make maintenance easier. If I understood correctly, this is the same technical solution previously discussed at the end of comment #3 and at the end of comment #4.

Ok, I was waiting for the green light to follow this path. I'll try to provide a rigorously formatted patch within 1 / 2 weeks.

(In reply to comment #7)
> Mathieu and I discussed this issue while attending WNS3 2012, here is the
> summary:
> 
> 1) the use case for having maps of objects is agreed upon, since there are
> several cases (in addition to the example posted by Jaume) where it is needed
> to provide to the user a list of objects with potentially non-consecutive IDs;
> 
> 2) that considered, Mathieu suggested to refactor the patch so as to always use
> maps under the hood, in order to avoid most of the code duplication and make
> maintenance easier. If I understood correctly, this is the same technical
> solution previously discussed at the end of comment #3 and at the end of
> comment #4.

(In reply to comment #7)
> Mathieu and I discussed this issue while attending WNS3 2012, here is the
> summary:
> 
> 1) the use case for having maps of objects is agreed upon, since there are
> several cases (in addition to the example posted by Jaume) where it is needed
> to provide to the user a list of objects with potentially non-consecutive IDs;
> 
> 2) that considered, Mathieu suggested to refactor the patch so as to always use
> maps under the hood, in order to avoid most of the code duplication and make
> maintenance easier. If I understood correctly, this is the same technical
> solution previously discussed at the end of comment #3 and at the end of
> comment #4.

bonus points go to a map implementation that handles strings as keys

Hi,

I've upload to codereview [1] the promised patch for the map attributes refactoring. I keept it quite minimal. Short list of the changes follows:
- Replaced the std::vector< Ptr<Object> > m_objects container by a std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support strings as well but I did not have much more time to test it, it goes to my to do buffer.
- Changed the virtual Ptr<Object> DoGet (const ObjectBase *object, uint32_t i) prototype to Ptr<Object> DoGet (const ObjectBase *object, uint32_t i, uint32_t *index) to get the index of the maps. For vectors, index and i are just the same but I could not find any other elegant solution to this.
- Changed the attribute iteration to support the new map container and the new Get API
- Extended the attribute testing to do some minor testing of this feature (actually it is a copy&paste&replace of the AttributeVector test, that was not extremely extensive).

On my local copy I tried it out with a couple of simulation programs and the patch did exactly what we needed.

If there is something else needed to be done, please, let me know.

Best regards,
Jaume

[http://codereview.appspot.com/6047044/]

(In reply to comment #10)

> - Replaced the std::vector< Ptr<Object> > m_objects container by a
> std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support
> strings as well but I did not have much more time to test it, it goes to my to
> do buffer.

If you feel confident that this can be done later, I am fine with it. The really nice thing to do would be to replace the Names implementation with this.

> [http://codereview.appspot.com/6047044/]

the patch looks really good. I will look at it again tomorrow before commit.

thanks for the hard work

(In reply to comment #11)
> (In reply to comment #10)
> 
> > - Replaced the std::vector< Ptr<Object> > m_objects container by a
> > std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support
> > strings as well but I did not have much more time to test it, it goes to my to
> > do buffer.
> 
> If you feel confident that this can be done later, I am fine with it. The
> really nice thing to do would be to replace the Names implementation with this.
> 
> > [http://codereview.appspot.com/6047044/]
> 
> the patch looks really good. I will look at it again tomorrow before commit.
> 
> thanks for the hard work


I'm recommending (from the code review comments) to:
- delete TestMap2 test attribute (unused and not understood)
- keep bug 1352 open for handling strings and also for resolving the question on attribute tests in the code review
- otherwise merge the latest patch so LENA merge can proceed

Jaume prepared a patch following Tom's indication, I just pushed it to ns-3-dev (changeset:   7884:963d5bfe9c52)

As a final comment, work pending to be done:
- String support
- Test getter and setter for vector and maps

I am sorry but was not able to work on this lately. I will try my best for the next release.

(In reply to comment #13)
> Jaume prepared a patch following Tom's indication, I just pushed it to ns-3-dev
> (changeset:   7884:963d5bfe9c52)

This is crashing under valgrind; to reproduce:
./test.py -g -s attributes

changeset:   8751:b89660102b63
parent:      8749:4462ac63d4cf
user:        Jaume Nin
date:        Mon May 14 18:15:22 2012 +0200
summary:     Fixed valgrind errors in attribute system. Fixed typo in AttributeMap test. Fixed Get() method for elements not present in the map

With
changeset:   8751:b89660102b63
parent:      8749:4462ac63d4cf
The bug should have been addressed

Let me know if it is not the case.

closing because no other issues have been raised since the commits from May 2012