Bug 86

Summary: add missing functions to Mac48Address code
Product: ns-3 Reporter: Mathieu Lacage <mathieu.lacage>
Component: networkAssignee: ns-bugs <ns-bugs>
Status: RESOLVED FIXED    
Severity: blocker CC: gjcarneiro
Priority: P1    
Version: pre-release   
Hardware: PC   
OS: Linux   

Description Mathieu Lacage 2007-10-09 04:50:11 UTC 
the two patchsets below add 3 new methods to Mac48Address:

bool Mac48Address::IsBroadcast (void) const;
bool Mac48Address::IsMulticast (void) const;
bool operator < (const Mac48Address &a, const Mac48Address &b);

http://code.nsnam.org/mathieu/ns-3-wifi/rev/88023edeb6ea
http://code.nsnam.org/mathieu/ns-3-wifi/rev/34f117b7a03b

May I merge these two patches into the trunk ?
Comment 1 Gustavo J. A. M. Carneiro 2007-10-09 09:27:39 UTC 
+bool operator < (const Mac48Address &a, const Mac48Address &b)
+{
+ uint8_t aP[6];
+ uint8_t bP[6];
+ a.CopyTo (aP);
+ b.CopyTo (bP); 

Isn't it easier/faster to compare directly m_data of a and b?  I think you can do it if only operator < is a member of the class... (in C++ you can access private members of other instances of the same class!).
Comment 2 Mathieu Lacage 2007-10-09 14:44:09 UTC 
(In reply to comment #1)
> +bool operator < (const Mac48Address &a, const Mac48Address &b)
> +{
> + uint8_t aP[6];
> + uint8_t bP[6];
> + a.CopyTo (aP);
> + b.CopyTo (bP); 
> 
> Isn't it easier/faster to compare directly m_data of a and b?  I think you can
> do it if only operator < is a member of the class... (in C++ you can access
> private members of other instances of the same class!).

you can make the non-member function be a friend of the class. But, clearly, the goal was not performance here.

>
Comment 3 Gustavo J. A. M. Carneiro 2007-10-10 06:58:14 UTC 
(In reply to comment #2)
> (In reply to comment #1)
> > +bool operator < (const Mac48Address &a, const Mac48Address &b)
> > +{
> > + uint8_t aP[6];
> > + uint8_t bP[6];
> > + a.CopyTo (aP);
> > + b.CopyTo (bP); 
> > 
> > Isn't it easier/faster to compare directly m_data of a and b?  I think you can
> > do it if only operator < is a member of the class... (in C++ you can access
> > private members of other instances of the same class!).
> 
> you can make the non-member function be a friend of the class. But, clearly,
> the goal was not performance here.

I have no proof of this, but I think "operator <" is going to be performance sensitive because it is used by stl::map and stl::set, for example.

I have another idea; why not make Mac48Address store a uint64_t internally?  This way comparison could become very fast on 64 bit processors, and probably also even in 32 bit ones (compiler optimisation).  That is assuming support for 64 bit integers is portable, of course (I have no idea).
Comment 4 Mathieu Lacage 2007-10-10 07:06:47 UTC 
> > you can make the non-member function be a friend of the class. But, clearly,
> > the goal was not performance here.

I will do the above to avoid the extra copy.

> 
> I have no proof of this, but I think "operator <" is going to be performance
> sensitive because it is used by stl::map and stl::set, for example.
> 
> I have another idea; why not make Mac48Address store a uint64_t internally? 

If we ever get into a performance bottleneck, we will optimize the code and use a uint64_t if it turns out to be really faster. However, until this happens, let's not make the implementation unreadable.

> This way comparison could become very fast on 64 bit processors, and probably
> also even in 32 bit ones (compiler optimisation).  That is assuming support for
> 64 bit integers is portable, of course (I have no idea).

We already decided that it was portable enough for us since we use it in a lot of places.
Comment 5 Tom Henderson 2007-10-10 10:38:05 UTC 
OK by me to commit.