[Ns-bugs] [Bug 1170] Formulate best practices for dealing with unused debug variables

[code@nsnam.ece.gatech.edu]

https://www.nsnam.org/bugzilla/show_bug.cgi?id=1170

--- Comment #44 from Andrey Mazo <mazo at iitp.ru> 2012-02-15 14:41:50 EST ---
(In reply to comment #43)
> (In reply to comment #42)
> > +1.  Could you please update the coding style accordingly?  (there was a draft
> > change proposed in an earlier comment that could be updated based on the
> > latest).
> 
> I've pushed the patch as changeset aef733235832.
> I'm updating the proposal and hope to finish it up tomorrow.

Please, comment on the proposal (sorry for my English beforehand).


Proposed new section to
http://www.nsnam.org/developers/contributing-code/coding-style/:

Writing debugging code
----------------------

ns-3 provides several macros used for debug builds, the main ones being
NS_ASSERT() and NS_LOG() and their variants.  Arguments of NS_ASSERT() and
NS_LOG() expand to nothing in optimized builds.  Therefore often, the code
around these macros triggers unused variable warnings/errors (and similar
errors) in optimized builds.  This section describes the coding style
and best practices around the use of debugging code.  We prefer to fix
these problems as they arise, without resorting to gcc flags or
attribute_unused directives, to maximize compatibility with other compilers.

The general rule is:
"""
no debug-related code should execute in the optimized build.
"""
The only exception from this rule is examples and tests.  Usually, they are
not performance-critical, so it is allowed (though not very welcomed) to have
some debugging code in them if that saves from ugly #ifdef/#else/#endif mess.


The implementation of NS_ASSERT() and NS_LOG() is different with NS_ASSERT()
being quieter in respect of compiler warnings.  This means that some
log-related code would produce warnings while the same assert-related code
would not.  Due to the fact, the following guidelines are mostly apply to
NS_LOG()-related code.  They could be painlessly applied to
NS_ASSERT()-related code though.


1) for simple NS_LOG and NS_ASSERT statements, avoid unnecessary
declarations and try to make the statement self-contained.  For example,
prefer

   NS_ASSERT (obj->GetCount () == 0);
to
   uint8_t tmp = obj->GetCount ();
   NS_ASSERT (tmp == 0);

Please, note that obj->GetCount () has no side-effect and thus can be
completely omitted in optimized builds.

2) complex debug-only code around NS_ASSERT() and NS_LOG() should be factored
out into a separate non-static global function.  The preferred way is to make
such a function the way its return value could be passed directly to
NS_LOG() or NS_ASSERT().  For example, the code 

void PacketSink::HandleRead (Ptr<Socket> socket)
{
  Ptr<Packet> packet;
  Address from;
  while (packet = socket->RecvFrom (from))
    {
      if (InetSocketAddress::IsMatchingType (from))
        {
          m_totalRx += packet->GetSize ();
          InetSocketAddress address = InetSocketAddress::ConvertFrom (from);
          NS_LOG_INFO ("Received " << packet->GetSize () << " bytes from "
                                   << address.GetIpv4 () << " [" << address <<
"]"
                                   << " total Rx " << m_totalRx);
        }
    }
}


should be refactored to


std::string PrintStats (Address& from, uint32_t packetSize, uint32_t
totalRxSize)
{
  std::ostringstream oss;
  InetSocketAddress address = InetSocketAddress::ConvertFrom (from);
  oss << "Received " <<  packetSize << " bytes from "
      << address.GetIpv4 () << " [" << address << "]"
      << " total Rx " << totalRxSize;

  return oss.str ();
}

void PacketSink::HandleRead (Ptr<Socket> socket)
{
  Ptr<Packet> packet;
  Address from;
  while (packet = socket->RecvFrom (from))
    {
      if (InetSocketAddress::IsMatchingType (from))
        {
          m_totalRx += packet->GetSize ();
          NS_LOG_INFO (PrintStats (from, packet->GetSize (), m_totalRx));
        }
    }
}


3) whenever it's impossible to apply approach 2), the NS_LOG() and NS_ASSERT()
related code with themselves should be factored out into a separate non-static
global function returning void. See
src/topology-read/model/rocketfuel-topology-reader.cc as an example.  The code

  NS_LOG_INFO ("Load Node[" << uid << "]: location: " << loc << " dns: " << dns
                            << " bb: " << bb << " neighbors: " <<
neigh_list.size ()
                            << "(" << "%d" << ") externals: \"%s\"(%d) "
                            << "name: " << name << " radius: " << radius);
  (void) bb;
  (void) dns;

should be

void
PrintNodeInfo (std::string & uid, std::string & loc, bool dns, bool bb,
               std::vector <std::string>::size_type neighListSize,
               std::string & name, int radius)
{
  NS_LOG_INFO ("Load Node[" << uid << "]: location: " << loc << " dns: " << dns
                            << " bb: " << bb << " neighbors: " << neighListSize
                            << "(" << "%d" << ") externals: \"%s\"(%d) "
                            << "name: " << name << " radius: " << radius);
}

 [...snip...]

  PrintNodeInfo (uid, loc, dns, bb, neigh_list.size (), name, radius);

3) use the NS_UNUSED() macro (which expands to a cast to (void)) for
any other cases where an unused variable warning needs silenced (such as
special uses of functions declared with warn_unused_result).

-- 
Configure bugmail: https://www.nsnam.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.