Created an attachment (id=368) [details]
ns-3-dev icc fixups

Added second patch.

(From update of attachment 368 [details])
Eh ? The following change can't possibly be right: what is the error message
output by icc ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
>--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
>@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> 
> template <typename T>
> Ptr<T> 
>-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
>-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
>-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
>-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
>-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
>-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
>-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
>-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
>-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
>+CreateObject (std::string n1 , const AttributeValue & v1,
>+              std::string n2 , const AttributeValue & v2,
>+              std::string n3 , const AttributeValue & v3,
>+              std::string n4 , const AttributeValue & v4,
>+              std::string n5 , const AttributeValue & v5,
>+              std::string n6 , const AttributeValue & v6,
>+              std::string n7 , const AttributeValue & v7,
>+              std::string n8 , const AttributeValue & v8,
>+              std::string n9 , const AttributeValue & v9)
> {
>   AttributeList attributes;
>   if (n1 == "")

What warning are you fixing below ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/internet-stack/ipv4-l3-protocol.cc
>--- a/src/internet-stack/ipv4-l3-protocol.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/internet-stack/ipv4-l3-protocol.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -1068,8 +1068,8 @@ Ipv4L3Protocol::SetUp (uint32_t i)
>   // If interface address and network mask have been set, add a route
>   // to the network of the interface (like e.g. ifconfig does on a
>   // Linux box)
>-  if ((interface->GetAddress ()) != (Ipv4Address ())
>-      && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>+  if ( ! interface->GetAddress ().IsEqual (Ipv4Address ())
>+       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>     {
>       AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),
>                          interface->GetNetworkMask (), i);

This is not really a warning fix, right ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/node/queue.cc
>--- a/src/node/queue.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/node/queue.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -84,12 +84,12 @@ Queue::Dequeue (void)
> 
>   if (packet != 0)
>     {
>+      NS_ASSERT (m_nBytes >= packet->GetSize ());
>+      NS_ASSERT (m_nPackets > 0);
>+
>       m_nBytes -= packet->GetSize ();
>       m_nPackets--;
> 
>-      NS_ASSERT (m_nBytes >= 0);
>-      NS_ASSERT (m_nPackets >= 0);
>-
>       NS_LOG_LOGIC("m_traceDequeue (packet)");
> 
>       m_traceDequeue (packet);

What warning are you fixing below ? LLU is not very portable so, adding:
uint64_t pow10 (uint8_t n)
{
  NS_ASSERT (n > 0);
  uint64_t result = 1;
  for (uint8_t i = 0; i < n; i++)
    {
      result *= 10;
    }
  return result;
}
would be probably more portable.

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/simulator/time.cc
>--- a/src/simulator/time.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/simulator/time.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -32,11 +32,11 @@ namespace ns3 {
> 
> namespace TimeStepPrecision {
> 
>-static const uint64_t MS_FACTOR = (uint64_t)pow(10,3);
>-static const uint64_t US_FACTOR = (uint64_t)pow(10,6);
>-static const uint64_t NS_FACTOR = (uint64_t)pow(10,9);
>-static const uint64_t PS_FACTOR = (uint64_t)pow(10,12);
>-static const uint64_t FS_FACTOR = (uint64_t)pow(10,15);
>+static const uint64_t MS_FACTOR = 1000LLU;
>+static const uint64_t US_FACTOR = 1000000LLU;
>+static const uint64_t NS_FACTOR = 1000000000LLU;
>+static const uint64_t PS_FACTOR = 1000000000000LLU;
>+static const uint64_t FS_FACTOR = 1000000000000000LLU;
> static uint64_t g_tsPrecFactor = NS_FACTOR;
> 
> static GlobalValue g_precisionDefaultValue ("TimeStepPrecision", 

Yes, sorry, some are warnings some are errors.

(In reply to comment #2)
> (From update of attachment 368 [details] [details])
> Eh ? The following change can't possibly be right: what is the error message
> output by icc ?

This is about the default arguments, they should only be declared in the
function declaration.

../src/core/object.h(406): warning #845: specifying a default argument when
redeclaring an unreferenced function template is nonstandard
  CreateObject (std::string n1 = "", const AttributeValue & v1 =
EmptyAttributeValue (),
  ^

../src/core/object.h(406): warning #350: redefinition of default argument
  CreateObject (std::string n1 = "", const AttributeValue & v1 =
EmptyAttributeValue (),
  ^

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
> >--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
> >@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> > 
> > template <typename T>
> > Ptr<T> 
> >-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
> >-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
> >-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
> >-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
> >-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
> >-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
> >-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
> >-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
> >-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
> >+CreateObject (std::string n1 , const AttributeValue & v1,
> >+              std::string n2 , const AttributeValue & v2,
> >+              std::string n3 , const AttributeValue & v3,
> >+              std::string n4 , const AttributeValue & v4,
> >+              std::string n5 , const AttributeValue & v5,
> >+              std::string n6 , const AttributeValue & v6,
> >+              std::string n7 , const AttributeValue & v7,
> >+              std::string n8 , const AttributeValue & v8,
> >+              std::string n9 , const AttributeValue & v9)
> > {
> >   AttributeList attributes;
> >   if (n1 == "")
> 
> What warning are you fixing below ?

It's an error, which I cannot tell you why it occurs:

../src/internet-stack/ipv4-l3-protocol.cc(1071): error: cast to type
"ns3::Ipv4Address ()" is not allowed
    if ((interface->GetAddress ()) != (Ipv4Address ())
                                       ^

../src/internet-stack/ipv4-l3-protocol.cc(1072): error: expected an expression
        && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
        ^

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/internet-stack/ipv4-l3-protocol.cc
> >--- a/src/internet-stack/ipv4-l3-protocol.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/internet-stack/ipv4-l3-protocol.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -1068,8 +1068,8 @@ Ipv4L3Protocol::SetUp (uint32_t i)
> >   // If interface address and network mask have been set, add a route
> >   // to the network of the interface (like e.g. ifconfig does on a
> >   // Linux box)
> >-  if ((interface->GetAddress ()) != (Ipv4Address ())
> >-      && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
> >+  if ( ! interface->GetAddress ().IsEqual (Ipv4Address ())
> >+       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
> >     {
> >       AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),
> >                          interface->GetNetworkMask (), i);
> 
> This is not really a warning fix, right ?

Warning about comparision of unsigned int >= 0. I changed the code to actually
compare something.

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/node/queue.cc
> >--- a/src/node/queue.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/node/queue.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -84,12 +84,12 @@ Queue::Dequeue (void)
> > 
> >   if (packet != 0)
> >     {
> >+      NS_ASSERT (m_nBytes >= packet->GetSize ());
> >+      NS_ASSERT (m_nPackets > 0);
> >+
> >       m_nBytes -= packet->GetSize ();
> >       m_nPackets--;
> > 
> >-      NS_ASSERT (m_nBytes >= 0);
> >-      NS_ASSERT (m_nPackets >= 0);
> >-
> >       NS_LOG_LOGIC("m_traceDequeue (packet)");
> > 
> >       m_traceDequeue (packet);
> 
> What warning are you fixing below ? LLU is not very portable so, adding:
> uint64_t pow10 (uint8_t n)
> {
>   NS_ASSERT (n > 0);
>   uint64_t result = 1;
>   for (uint8_t i = 0; i < n; i++)
>     {
>       result *= 10;
>     }
>   return result;
> }
> would be probably more portable.

Interesting to talk about portability between compilers here ;).

This is actually a runtime error. Any example program will segfault, because
these static consts are incorrectly initialized, gdb says they are 0 at
segfault time.

Your idea with pow10 did not work.

The issue is about initialization order: in my dump WifiMac::GetDefaultSifs()
is called during initialization _before_ the pow10() is calculated. SIFS is
returned with MicroSeconds(), which uses one of the factors.

static const uint64_t MS_FACTOR = (uint64_t)1000;
static const uint64_t US_FACTOR = (uint64_t)1000000;
static const uint64_t NS_FACTOR = (uint64_t)1000000 * (uint64_t)1000;
static const uint64_t PS_FACTOR = (uint64_t)1000000 * (uint64_t)1000000;
static const uint64_t FS_FACTOR = (uint64_t)1000000 * (uint64_t)1000000 *
(uint64_t)1000;

The above works. The compilers (gcc/icc) seem to be able to fold the
expression.

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/simulator/time.cc
> >--- a/src/simulator/time.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/simulator/time.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -32,11 +32,11 @@ namespace ns3 {
> > 
> > namespace TimeStepPrecision {
> > 
> >-static const uint64_t MS_FACTOR = (uint64_t)pow(10,3);
> >-static const uint64_t US_FACTOR = (uint64_t)pow(10,6);
> >-static const uint64_t NS_FACTOR = (uint64_t)pow(10,9);
> >-static const uint64_t PS_FACTOR = (uint64_t)pow(10,12);
> >-static const uint64_t FS_FACTOR = (uint64_t)pow(10,15);
> >+static const uint64_t MS_FACTOR = 1000LLU;
> >+static const uint64_t US_FACTOR = 1000000LLU;
> >+static const uint64_t NS_FACTOR = 1000000000LLU;
> >+static const uint64_t PS_FACTOR = 1000000000000LLU;
> >+static const uint64_t FS_FACTOR = 1000000000000000LLU;
> > static uint64_t g_tsPrecFactor = NS_FACTOR;
> > 
> > static GlobalValue g_precisionDefaultValue ("TimeStepPrecision", 
> 

Created an attachment (id=369) [details]
ns-3-dev icc fixups

Updated patch.

(In reply to comment #0)
> Created an attachment (id=367) [details] [details]
> icc flags for waf
> 
> These two patches enable compilation with icc.
> 
> The new waf has support for icpc, but the cflags extension in our patched waf
> does not. See waf-gjc-icc-cflags.patch for that. The patch does not add -Wall
> to CXXFLAGS because it makes icc very verbose.
> 

To be clear, the first patch isn't for ns-3-dev, right?  You are saying that it
is a patch against Gustavo's fork of waf?
https://code.launchpad.net/~gjc/waf/cmd

If so, the first patch is orthogonal to ns-3.  I suggest Gustavo decide what to
do with it (i.e. whether or not to merge it into his waf fork).  After doing
so, he can generate a new standalone waf and include this in ns-3-dev.

From the perspective of ns-3, compilation with icc should probably be a not
officially supported feature (simply a feature of waf, which we happen to use),
meaning that the build could break anytime if you are using icc.  That said, if
changes like the second patch are proposed for ns-3 which don't regress the gcc
builds and generally make our code more standards compliant / cleaner, we
should accept them.

Right on.

Yes the first patch is for Gustavo's waf.

icc is farly compatible with g++. The errors and warning it outputs generally
lead to higher code quality.

(From update of attachment 367 [details])
applied to the waf/cmd tree on launchpad, will later sync to ns-3-dev's waf

(From update of attachment 369 [details])
>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
>--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
>@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> 
> template <typename T>
> Ptr<T> 
>-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
>-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
>-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
>-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
>-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
>-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
>-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
>-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
>-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
>+CreateObject (std::string n1 , const AttributeValue & v1,
>+              std::string n2 , const AttributeValue & v2,
>+              std::string n3 , const AttributeValue & v3,
>+              std::string n4 , const AttributeValue & v4,
>+              std::string n5 , const AttributeValue & v5,
>+              std::string n6 , const AttributeValue & v6,
>+              std::string n7 , const AttributeValue & v7,
>+              std::string n8 , const AttributeValue & v8,
>+              std::string n9 , const AttributeValue & v9)
> {
>   AttributeList attributes;
>   if (n1 == "")


This chunk worries me.  You are removing the default parameter values; how does
this not break existing code?

(In reply to comment #8)
> 
> 
> This chunk worries me.  You are removing the default parameter values; how does
> this not break existing code?
> 

As far as I know, the standard says that default parameters should only go in
the signature of the specification of the method, not in the implementation. 
Although putting them in the implementation signature improves readability (and
GCC allows it), it is not C++ standard.

(In reply to comment #8)
> 
> This chunk worries me.  You are removing the default parameter values; how does
> this not break existing code?
> 

Don't look at just the diff. The default parameters are declared in the
function's declaration a few lines above. They should not be repeated in the
function's definition.

(In reply to comment #3)

> > What warning are you fixing below ?
> 
> It's an error, which I cannot tell you why it occurs:
> 
> ../src/internet-stack/ipv4-l3-protocol.cc(1071): error: cast to type
> "ns3::Ipv4Address ()" is not allowed
>     if ((interface->GetAddress ()) != (Ipv4Address ())
>                                        ^
> ../src/internet-stack/ipv4-l3-protocol.cc(1072): error: expected an expression
>         && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>         ^

I stepped on this bug while compiling ns-3 with g++ 4.4 snapshot:

../src/internet-stack/ipv4-l3-protocol.cc: In member function ‘void
ns3::Ipv4L3Protocol::SetUp(uint32_t)’:
../src/internet-stack/ipv4-l3-protocol.cc:1072: error: expected identifier
before ‘(’ token
../src/internet-stack/ipv4-l3-protocol.cc:1072: warning: taking the address of
a label is non-standard
../src/internet-stack/ipv4-l3-protocol.cc:1072: error: expected ‘)’ before ‘(’
token

icc is more informative here; at least it tells us that it expects something to
be casted to Ipv4Mask() (how did it get that idea?). Adding another pair of
parentheses quieted the compiler:

--- ns-3-dev/src/internet-stack/ipv4-l3-protocol.cc    2009-02-14
03:53:06.000000000 +0300
+++ ns-3-rev/src/internet-stack/ipv4-l3-protocol.cc    2009-02-14
03:45:42.000000000 +0300
@@ -1068,7 +1068,7 @@
   // If interface address and network mask have been set, add a route
   // to the network of the interface (like e.g. ifconfig does on a
   // Linux box)
-  if ((interface->GetAddress ()) != (Ipv4Address ())
+  if (((interface->GetAddress ()) != (Ipv4Address ()))
       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
     {
       AddNetworkRouteTo (interface->GetAddress ().CombineMask
(interface->GetNetworkMask ()),

changeset da9be6abb1b2

same as last patch, except with change from Anatoly Kamchatnov