Bug 284

Summary: cannot use config paths to get a handle on an object
Product: ns-3 Reporter: Mathieu Lacage <mathieu.lacage>
Component: coreAssignee: Mathieu Lacage <mathieu.lacage>
Status: RESOLVED FIXED    
Severity: normal CC: gjcarneiro, ns-bugs, raj.b
Priority: P3    
Version: pre-release   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 280    
Attachments: proposed patch
update

Description Mathieu Lacage 2008-08-11 19:27:19 UTC
some users asked me today why it was not possible to use /NodeList/x/DeviceList/y to get access to a specific device.

Of course, the answer is that there is not necessarily a single match for a specific string so, we would have to provide this:

namespace Config {

std::vector<Ptr<Object> > LookupMatching (std::string path);

}
Comment 1 Tom Henderson 2008-08-12 00:30:48 UTC
This seems like an intuitive feature to offer (also for being able to check the existence of the device), so I would support adding it.
Comment 2 Mathieu Lacage 2008-08-12 11:34:47 UTC
While thinking about this some more tonight, it occured to me that we could use this to solve partly our error-reporting problems in the Config API. What about something like this:

class MatchContainer
{
public:
  Ptr<Object> Get (uint32_t i) const;
  uint32_t GetN (void) const;
  std::string GetPath (uint32_t i) const;

  void Connect (std::string traceSource, const CallbackBase &);
  void Set (std::string attribute, const AttributeValue &v);
private:
  std::vector<std::pair<Ptr<Object>,std::string> > m_matches;
};

namespace Config {

MatchContainer LookupMatches (std::string path);

}
Comment 3 Rajib Bhattacharjea 2008-08-12 11:41:09 UTC
(In reply to comment #0)
> some users asked me today why it was not possible to use
> /NodeList/x/DeviceList/y to get access to a specific device.
> 
> Of course, the answer is that there is not necessarily a single match for a
> specific string so, we would have to provide this:
> 
> namespace Config {
> 
> std::vector<Ptr<Object> > LookupMatching (std::string path);
> 
> }
> 

It seems to me that there are four types of things the config strings can specify: trace sources, nodes, devices, and things aggregated to a node (I feel like I'm forgetting some other use of these though...).  In the specific cases of nodes or devices, it might be nice to have different APIs that return a more specific container:

namespace Config {

NodeContainer LookupMatchingNodes (std::string path);
NetDeviceContainer LookupMatchingNetDevices (std::string path);
std::vector<Ptr<Object> > LookupMatchingObjects (std::string path);

}

And I can't think of why anyone would want to directly get a handle to a TracedValue or TracedCallback, so perhaps this should be disallowed?
Comment 4 Mathieu Lacage 2008-08-12 11:51:37 UTC
(In reply to comment #3)
> It seems to me that there are four types of things the config strings can
> specify: trace sources, nodes, devices, and things aggregated to a node (I feel
> like I'm forgetting some other use of these though...).  In the specific cases

yes. There are many more objects which can be accessed through the config API. Basically, anything which derives from Object.

> of nodes or devices, it might be nice to have different APIs that return a more
> specific container:
> 
> namespace Config {
> 
> NodeContainer LookupMatchingNodes (std::string path);
> NetDeviceContainer LookupMatchingNetDevices (std::string path);
> std::vector<Ptr<Object> > LookupMatchingObjects (std::string path);
> 
> }
> 
> And I can't think of why anyone would want to directly get a handle to a
> TracedValue or TracedCallback, so perhaps this should be disallowed?

There is no need to disallow it: it is simply impossible because these are not subclasses of the Object base class so, they cannot be returned in an array of Ptr<Object>.



Comment 5 Tom Henderson 2008-08-26 01:02:03 UTC
(In reply to comment #2)
> While thinking about this some more tonight, it occured to me that we could use
> this to solve partly our error-reporting problems in the Config API. What about
> something like this:
> 
> class MatchContainer
> {
> public:
>   Ptr<Object> Get (uint32_t i) const;
>   uint32_t GetN (void) const;
>   std::string GetPath (uint32_t i) const;
> 
>   void Connect (std::string traceSource, const CallbackBase &);
>   void Set (std::string attribute, const AttributeValue &v);
> private:
>   std::vector<std::pair<Ptr<Object>,std::string> > m_matches;
> };
> 
> namespace Config {
> 
> MatchContainer LookupMatches (std::string path);
> 
> }
> 

again, this looks like a good idea to me
Comment 6 Mathieu Lacage 2008-10-20 06:39:42 UTC
Created attachment 271 [details]
proposed patch
Comment 7 Mathieu Lacage 2008-10-20 07:25:34 UTC
Created attachment 272 [details]
update
Comment 8 Mathieu Lacage 2008-10-20 08:06:20 UTC
*** Bug 164 has been marked as a duplicate of this bug. ***
Comment 9 Gustavo J. A. M. Carneiro 2008-10-20 10:57:30 UTC
I hate the name MatchContainer.  It has negative information value (confuses the reader more than clarifies, it sounds like more an object to hold the results of a regular expression match operation).

I think something like NamedObjectList is more clear.

But I still think supporting names in NodeList is by far the option that makes most sense IMHO.
Comment 10 Mathieu Lacage 2008-10-21 02:14:47 UTC
(In reply to comment #9)
> I hate the name MatchContainer.  It has negative information value (confuses
> the reader more than clarifies, it sounds like more an object to hold the
> results of a regular expression match operation).

Which is exactly that.

> 
> I think something like NamedObjectList is more clear.

I think that you are confused. This is really a regexp match and we return the result of the match.


> 
> But I still think supporting names in NodeList is by far the option that makes
> most sense IMHO.

That is a different issue, unrelated to this bug.

Comment 11 Gustavo J. A. M. Carneiro 2008-10-21 05:57:16 UTC
(In reply to comment #10)
> I think that you are confused. This is really a regexp match and we return the
> result of the match.

Righ, I was confused! That's what you get for reading bug reports in a hurry... :P  So never mind my previous comments.

So, looking at this more closely, I like it very much.  Only two comments:

  1. >   void Set (std::string attribute, const AttributeValue &v);
    I would prefer SetAttribute here, or maybe SetAttributeForeach.  It took me a few seconds to figure out what this 'Set' was supposed to do, until I figured out it must be "set an attribute on the object(s) pointed to by the trace path.  Also it makes it more consistent with ObjectBase::SetAttribute;

  2. This is a great opportunity to factor in a fix for bug #127 ;-) i.e. provide a way to obtain the callback signature for a trace path.  So, if you could add a method like const std::type_info & GetCallbackType (), or GetCallbackType (int i), it should be enough to get tracing working for Python bindings, with some additional effort on my part.
Comment 12 Mathieu Lacage 2008-10-21 06:28:59 UTC
(In reply to comment #11)

> So, looking at this more closely, I like it very much.  Only two comments:
> 
>   1. >   void Set (std::string attribute, const AttributeValue &v);
>     I would prefer SetAttribute here, or maybe SetAttributeForeach.  It took me
> a few seconds to figure out what this 'Set' was supposed to do, until I figured
> out it must be "set an attribute on the object(s) pointed to by the trace path.
>  Also it makes it more consistent with ObjectBase::SetAttribute;

I agree but I tried to be consistent with Config::Set :/


>   2. This is a great opportunity to factor in a fix for bug #127 ;-) i.e.
> provide a way to obtain the callback signature for a trace path.  So, if you
> could add a method like const std::type_info & GetCallbackType (), or
> GetCallbackType (int i), it should be enough to get tracing working for Python
> bindings, with some additional effort on my part.

how about you implement it in a branch, and, then, I will review the changes you needed to make to the callback code ?
Comment 13 Gustavo J. A. M. Carneiro 2008-10-21 06:51:40 UTC
(In reply to comment #12)
> (In reply to comment #11)
> >   2. This is a great opportunity to factor in a fix for bug #127 ;-) i.e.
> > provide a way to obtain the callback signature for a trace path.  So, if you
> > could add a method like const std::type_info & GetCallbackType (), or
> > GetCallbackType (int i), it should be enough to get tracing working for Python
> > bindings, with some additional effort on my part.
> 
> how about you implement it in a branch, and, then, I will review the changes
> you needed to make to the callback code ?
> 

OK, but I probably won't get to it any time soon.  I remember I had a working patch once, but then NS 3 tracing was completely refactored and the patch is now useless.  But I remember how hard it was to produce the patch, as it has very complicated C++ code in there.  I don't really need tracing in python for my PhD work, so this is very low priority for me...
Comment 14 Tom Henderson 2008-10-23 00:29:18 UTC
(In reply to comment #7)
> Created an attachment (id=272) [details]
> update
> 

+1

Comment 15 Mathieu Lacage 2008-10-24 06:38:53 UTC
changeset 985324e2caaa