Bugzilla – Bug 1170
Formulate best practices for dealing with unused debug variables
Last modified: 2014-03-31 04:52:15 EDT
Formulate best practices for dealing with unused debug variables (i.e variable set but not used compiler warning in optimized builds)
(In reply to comment #0) > Formulate best practices for dealing with unused debug variables (i.e variable > set but not used compiler warning in optimized builds) Some discussion took place in thread [1]. [1] http://mailman.isi.edu/pipermail/ns-developers/2011-May/009021.html
Created attachment 1154 [details] NS_UNUSED() macro
This is an attempt to try to summarize consensus? about best practices. Please comment so we can try to close these. Proposed best practices: 1) ns-3 does not want to disable these warnings but fix them as they arise 2) debug code should not execute in the optimized build. 3) try to fix problems as they arise without resorting to casting to void* or attribute_unused 4) avoid unnecessary declarations to feed to LOG or ASSERT statements 5) for multiline debug code, wrap around something like #ifdef NS3_ASSERT_ENABLE or #ifdef NS3_LOG_ENABLE This would avoid introducing NS_UNUSED(). Avoid unnecessary declarations such as follows: uint8_t tmp = start.ReadU8 (); // major version of radiotap header NS_ASSERT_MSG (tmp == 0x00, "RadiotapHeader::Deserialize(): Unexpected major version"); NS_UNUSED (tmp); would become just: NS_ASSERT_MSG (start.ReadU8 () == 0x00, "RadiotapHeader::Deserialize(): Unexpected major version"); For cases in which the ASSERT/LOG statement gets too complicated or a declaration must be made, we would use multiline such as: #ifdef NS3_LOG_ENABLE InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " port: " << iaddr.GetPort ()); #endif
(In reply to comment #3) > This would avoid introducing NS_UNUSED(). Avoid unnecessary declarations such > as follows: > > uint8_t tmp = start.ReadU8 (); // major version of radiotap header > NS_ASSERT_MSG (tmp == 0x00, "RadiotapHeader::Deserialize(): Unexpected major > version"); > NS_UNUSED (tmp); > > would become just: > NS_ASSERT_MSG (start.ReadU8 () == 0x00, "RadiotapHeader::Deserialize(): > Unexpected major version"); I believe, these two blocks of code are not equal because start.ReadU8() has a side effect which will be missing in the second case. So we have to use start.Next() for optimized build which leads to #ifdef NS3_ASSERT_ENABLE/#else/#endif construct. > For cases in which the ASSERT/LOG statement gets too complicated or a > declaration must be made, we would use multiline such as: > > #ifdef NS3_LOG_ENABLE > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " > port: " << iaddr.GetPort ()); > #endif My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT statements into #ifdef/#else/#endif. We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT also use them internally.
(In reply to comment #4) > (In reply to comment #3) > > This would avoid introducing NS_UNUSED(). Avoid unnecessary declarations such > > as follows: > > > > uint8_t tmp = start.ReadU8 (); // major version of radiotap header > > NS_ASSERT_MSG (tmp == 0x00, "RadiotapHeader::Deserialize(): Unexpected major > > version"); > > NS_UNUSED (tmp); > > > > would become just: > > NS_ASSERT_MSG (start.ReadU8 () == 0x00, "RadiotapHeader::Deserialize(): > > Unexpected major version"); > I believe, these two blocks of code are not equal because start.ReadU8() has a > side effect which will be missing in the second case. > So we have to use start.Next() for optimized build which leads to #ifdef > NS3_ASSERT_ENABLE/#else/#endif construct. Probably the example can be better chosen. In the other hand that is just an example. Generally speaking, as is about the best practice, I do agree with what Tom wrote. > > For cases in which the ASSERT/LOG statement gets too complicated or a > > declaration must be made, we would use multiline such as: > > > > #ifdef NS3_LOG_ENABLE > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " > > port: " << iaddr.GetPort ()); > > #endif > > My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT > statements into #ifdef/#else/#endif. > We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT > also use them internally. To be honest duplicating #ifdef is not a problem, as the preprocessor will strip them and they don't collapse each other. On the contrary the NS_UNUSED macro *will* pass tru the preprocessor and it will be compiler's responsibility to strip it. From a purely aesthetic point of view, I'm more favorable to Tom's approach, as it points out that debug code should be cleaned out after... debugging and only the code really necessary for *future* development or really important stuff should be left in the code. Cheers, Tommaso
(In reply to comment #4) > (In reply to comment #3) > > This would avoid introducing NS_UNUSED(). Avoid unnecessary declarations such > > as follows: > > > > uint8_t tmp = start.ReadU8 (); // major version of radiotap header > > NS_ASSERT_MSG (tmp == 0x00, "RadiotapHeader::Deserialize(): Unexpected major > > version"); > > NS_UNUSED (tmp); > > > > would become just: > > NS_ASSERT_MSG (start.ReadU8 () == 0x00, "RadiotapHeader::Deserialize(): > > Unexpected major version"); > I believe, these two blocks of code are not equal because start.ReadU8() has a > side effect which will be missing in the second case. Agree, this was a bad example. I meant to put a no-side-effect case. > So we have to use start.Next() for optimized build which leads to #ifdef > NS3_ASSERT_ENABLE/#else/#endif construct. > > > For cases in which the ASSERT/LOG statement gets too complicated or a > > declaration must be made, we would use multiline such as: > > > > #ifdef NS3_LOG_ENABLE > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " > > port: " << iaddr.GetPort ()); > > #endif > > My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT > statements into #ifdef/#else/#endif. > We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT > also use them internally. I'm guessing that you are still on the position you stated in the list: - if side-effect is desired, use an NS_UNUSED() after to silence warnings - if multiline code has no side effect, wrap it within an ifdef/endif
(In reply to comment #5) > > My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT > > statements into #ifdef/#else/#endif. > > We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT > > also use them internally. > > To be honest duplicating #ifdef is not a problem, as the preprocessor will > strip them and they don't collapse each other. On the contrary the NS_UNUSED > macro *will* pass tru the preprocessor and it will be compiler's responsibility > to strip it. Of course, it's not a technical problem because preprocessor can easily deal with it. I just don't like duplicating the check from design point of view. And my complain is that to the moment NS3_ASSERT_ENABLE is kind of an internal private macro, not exposed to users. > From a purely aesthetic point of view, I'm more favorable to Tom's approach, as > it points out that debug code should be cleaned out after... debugging and only > the code really necessary for *future* development or really important stuff > should be left in the code. As far as I understand you and Tom, Tom is not proposing to clean out debugging code completely. He is just proposing not to compile it for optimized builds. I suppose debugging code is valuable for future developments too.
(In reply to comment #6) > > My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT > > statements into #ifdef/#else/#endif. > > We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT > > also use them internally. > > I'm guessing that you are still on the position you stated in the list: > - if side-effect is desired, use an NS_UNUSED() after to silence warnings If everyone agrees to prevent unused code from being compiled in optimized builds, thats ok to me too. But I would like to see a nice macro to hide #ifdef/#else/#endif. Something like: NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); which expands to (in debug build): uint8_t tmp = start.ReadU8 (); or (in optimized build): start.ReadU8 (); > - if multiline code has no side effect, wrap it within an ifdef/endif Right, but hide #ifdef/#endif behind NS_LOG_BEGIN; NS_LOG_END;
(In reply to comment #8) > (In reply to comment #6) > > > My point here is it becomes too verbose to wrap nearly all NS_LOG/NS_ASSERT > > > statements into #ifdef/#else/#endif. > > > We are kind of duplicating these #ifdef/#else/#endif because NS_LOG/NS_ASSERT > > > also use them internally. > > > > I'm guessing that you are still on the position you stated in the list: > > - if side-effect is desired, use an NS_UNUSED() after to silence warnings > If everyone agrees to prevent unused code from being compiled in optimized > builds, thats ok to me too. > But I would like to see a nice macro to hide #ifdef/#else/#endif. > Something like: > NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); > which expands to (in debug build): > uint8_t tmp = start.ReadU8 (); > or (in optimized build): > start.ReadU8 (); > > > - if multiline code has no side effect, wrap it within an ifdef/endif > Right, but hide #ifdef/#endif behind > NS_LOG_BEGIN; > NS_LOG_END; I guess that I'm neutral to slightly favoring just the explicit (slightly more verbose) ifdef convention, since it would be one less ns-3 idiom to learn. Introducing NS_LOG_ASSIGNMENT might mean that few ifdef/endif multiline blocks would remain.
Here is a revised proposal; will ask on ns-developers if this is an acceptable solution. 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. 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. 1) no debug-related code should execute in the optimized build. 2) 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); 3) some code needs to execute in an optimized environment, but the return value is only used in a debug environment. Wrap the declaration as follows: NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); which expands to (in debug build): uint8_t tmp = start.ReadU8 (); or (in optimized build): start.ReadU8 (); A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping assert-related temporary variables. 4) for multiline code, wrap the code block around these macros NS_LOG_BEGIN(); NS_LOG_END(); which expand to #ifdef/#endif constructs 5) 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).
(In reply to comment #10) > Here is a revised proposal; will ask on ns-developers if this is an acceptable > solution. +1
(In reply to comment #11) > (In reply to comment #10) > > Here is a revised proposal; will ask on ns-developers if this is an acceptable > > solution. > > +1 Seems like lazy consensus to proceed; Andrey volunteered to create the patch.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Here is a revised proposal; will ask on ns-developers if this is an acceptable > > > solution. > > > > +1 > > > Seems like lazy consensus to proceed; Andrey volunteered to create the patch. I know I am coming late to the discussion but I really don't see much point in introducing the new macros and hiding #ifdef. If you really need #ifdef, use it but use it sparingly. In the cases where you might need to use it a lot, you just need to restructure your code by creating helper functions. For example: > > #ifdef NS3_LOG_ENABLE > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " > > port: " << iaddr.GetPort ()); > > #endif Ipv4Address ToIpv4 (const Address &addr) { InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); return iaddr.GetIpv4 (); } Same for ToPort and finally: NS_LOG_DEBUG ("Received one packet! Socket: " << ToIpv4(addr) << " port: " << ToPort (addr)); The above is simple, low tech, fairly easy to read and is simply what everyone else has been doing forever in every other project. Why can't we do the same ?
(In reply to comment #13) > For example: > > > > #ifdef NS3_LOG_ENABLE > > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > > NS_LOG_UNCOND ("Received one packet! Socket: " << iaddr.GetIpv4 () << " > > > port: " << iaddr.GetPort ()); > > > #endif > > Ipv4Address ToIpv4 (const Address &addr) > { > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > return iaddr.GetIpv4 (); > } > > Same for ToPort > > and finally: > NS_LOG_DEBUG ("Received one packet! Socket: " << ToIpv4(addr) << " port: " << > ToPort (addr)); > > The above is simple, low tech, fairly easy to read and is simply what everyone > else has been doing forever in every other project. Why can't we do the same ? Well, this will make debug builds even slower. I'll try to do some rough performance measurements. Also, it makes sense to declare such helper functions as static which in turn will trigger "defined but not used" warnings in optimized builds.
Created attachment 1216 [details] Introduce NS_LOG_ASSIGNMENT, NS_ASSERT_ASSIGNMENT, NS_UNUSED. Introduce and/or make use of the following macros: NS3_LOG_ENABLE NS3_ASSERT_ENABLE NS_LOG_ASSIGNMENT NS_LOG_ONLY_ASSIGNMENT NS_ASSERT_ASSIGNMENT NS_ASSERT_ONLY_ASSIGNMENT NS_UNUSED
(In reply to comment #10) I've created the attachment 1216 [details] with proposed changes. Unfortunately, it has several differences compared to what was decided here. 1) I've added NS_{ASSERT,LOG}_ONLY_ASSIGMENT(var, value) in addition to NS_{ASSERT,LOG}_ASSIGMENT(var, value). The difference is that NS_{ASSERT,LOG}_ONLY_ASSIGMENT(var, value) are expanded to nothing in optimized build, while NS_{ASSERT,LOG}_ASSIGMENT(var, value) are expanded to (value). This is useful when debugging code is tightly interleaved with production code. Example usage can be found in attachment 1216 [details]. I don't like much the names NS_ASSERT_ONLY_ASSIGMENT and NS_LOG_ONLY_ASSIGMENT as they are not very intuitive to me, so any suggestions are welcome. 2) I haven't come up with a preprocessor-only solution to hide "#ifdef NS3_LOG_ENABLE" as NS_LOG_BEGIN(), so I've used NS3_LOG_ENABLE and NS3_ASSERT_ENABLE directly. Of course, it's possible to "#define NS_LOG_BEGIN() if(0) {" for optimized builds and "#define NS_LOG_BEGIN() if(1) {" for debugging builds, but that leaves the task of removing the dead code to a compiler. So, I've updated the proposal according to these changes. 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. 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. 1) no debug-related code should execute in the optimized build. 2) 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); 3) if unnecessary declarations are hard to avoid (e.g. helper function call and assert itself are delimited with a necessary function call), wrap the temporary variable and the helper function call as follows: NS_ASSERT_ONLY_ASSIGNMENT (uint32_t availableData, socket->GetRxAvailable ()); m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); NS_ASSERT (availableData == m_receivedPacket->GetSize ()); which expands in debug build to: uint32_t availableData = socket->GetRxAvailable (); m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); NS_ASSERT (availableData == m_receivedPacket->GetSize ()); and in optimized build to: m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); A similar macro NS_LOG_ONLY_ASSIGMENT() should be used for wrapping assert-related temporary variables. 4) some code needs to execute in an optimized environment, but the return value is only used in a debug environment. Wrap the declaration as follows: NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); which expands to (in debug build): uint8_t tmp = start.ReadU8 (); or (in optimized build): start.ReadU8 (); A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping assert-related temporary variables. 5) for multiline code, wrap debugging code into #ifdef NS3_LOG_ENABLE/#endif pair #ifdef NS3_LOG_ENABLE Ptr<Packet> packet; Address from; while (packet = socket->RecvFrom (from)) { if (packet->GetSize () > 0) { InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (from); NS_LOG_UNCOND ("Received one packet! Socket: "<< iaddr.GetIpv4 () << "\n"); } } #else while (socket->Recv ()); #endif A similar macro NS3_ASSERT_ENABLE should be used for wrapping assert-related temporary variables. 6) 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).
(In reply to comment #14) > > Ipv4Address ToIpv4 (const Address &addr) > > { > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > return iaddr.GetIpv4 (); > > } > > > > Same for ToPort > > > > and finally: > > NS_LOG_DEBUG ("Received one packet! Socket: " << ToIpv4(addr) << " port: " << > > ToPort (addr)); > > > > The above is simple, low tech, fairly easy to read and is simply what everyone > > else has been doing forever in every other project. Why can't we do the same ? > > Well, this will make debug builds even slower. Andrey, while the following argument is indeed a valid argument, the above sadly makes not much sense: - debug builds are not here to be fast. They are butt slow. period. If you look at the amount of checking I had added to Packet, you will know why. - what you refer to is a function that will be called only when a specific logging variable is set. When this happens, runtime is dominated by string formating operations, not a couple of extra function calls. And you know perfectly well all of that. > I'll try to do some rough performance measurements. > Also, it makes sense to declare such helper functions as static which in turn > will trigger "defined but not used" warnings in optimized builds. This is indeed a good point and it is true in C (although, really, from a simplicity point of view, I would be willing to live with non-static functions for that in C), but this is C++ so, there are a couple of classic tricks around this issue: 1) make the static function a member static method: no warnings anymore, even with recent gccs. 2) move the static function to an anonymous namespace. again, no warnings with any compiler I am aware of. To summarize, there are solutions to the real problem you highlight. So, I will have to NAK your followup patch proposal, unless you come up with better arguments against my proposal. (if you complain (rightfully) about my slowness to answer that comment, I have to agree and offer an apology)
(In reply to comment #16) > 1) I've added NS_{ASSERT,LOG}_ONLY_ASSIGMENT(var, value) in addition to > NS_{ASSERT,LOG}_ASSIGMENT(var, value). nack > 2) I haven't come up with a preprocessor-only solution to hide "#ifdef > NS3_LOG_ENABLE" as NS_LOG_BEGIN(), so I've used NS3_LOG_ENABLE and Yes, this is impossible. > 1) no debug-related code should execute in the optimized build. ok > > 2) 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); ok > > 3) if unnecessary declarations are hard to avoid (e.g. helper function > call and assert itself are delimited with a necessary function call), wrap the > temporary variable and the helper function call as follows: nack. introduce a helper function. > > NS_ASSERT_ONLY_ASSIGNMENT (uint32_t availableData, socket->GetRxAvailable > ()); > m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); > NS_ASSERT (availableData == m_receivedPacket->GetSize ()); > which expands in debug build to: > uint32_t availableData = socket->GetRxAvailable (); > m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); > NS_ASSERT (availableData == m_receivedPacket->GetSize ()); > and in optimized build to: > m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); > > A similar macro NS_LOG_ONLY_ASSIGMENT() should be used for wrapping > assert-related temporary variables. > > 4) some code needs to execute in an optimized environment, but the > return value is only used in a debug environment. Wrap the declaration > as follows: nack. introduce a helper function for the debug code, do as usual for the optimized code. > > NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); > which expands to (in debug build): > uint8_t tmp = start.ReadU8 (); > or (in optimized build): > start.ReadU8 (); > > A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping > assert-related temporary variables. > > 5) for multiline code, wrap debugging code into #ifdef NS3_LOG_ENABLE/#endif > pair nack. use a helper function. > #ifdef NS3_LOG_ENABLE > Ptr<Packet> packet; > Address from; > while (packet = socket->RecvFrom (from)) > { > if (packet->GetSize () > 0) > { > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (from); > NS_LOG_UNCOND ("Received one packet! Socket: "<< iaddr.GetIpv4 () > << "\n"); > } > } > #else > while (socket->Recv ()); > #endif > > A similar macro NS3_ASSERT_ENABLE should be used for wrapping > assert-related temporary variables. > > 6) 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). ok.
(In reply to comment #17) > > > Ipv4Address ToIpv4 (const Address &addr) > > > { > > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > > return iaddr.GetIpv4 (); > > > } > > > > > > Same for ToPort > > > > > > and finally: > > > NS_LOG_DEBUG ("Received one packet! Socket: " << ToIpv4(addr) << " port: " << > > > ToPort (addr)); > > > > This is indeed a good point and it is true in C (although, really, from a > simplicity point of view, I would be willing to live with non-static functions > for that in C), but this is C++ so, there are a couple of classic tricks around > this issue: > > 1) make the static function a member static method: no warnings anymore, even > with recent gccs. Considering the above ToIpv4() example, which class should we make the static method member of? I see two options: 1.1) Make ToIpv4() method a member of Ipv4Address class 1.2) Make ToIpv4() method a member of the class that uses it (like PacketSink in this example) Option 1.1) requires someone to modify the core class whenever he needs a new NS_LOG_*()/NS_ASSER() statement. Also this will be impossible when the app store idea gets implemented. Addition of helper functions to the core class will also introduce additional dependencies and will end up with everything depending on everything. Both options make the whole ns-3 API bloated with a bunch of (duplicated) helper functions, occupying more space in dynamic symbol table, which adds more work to both the linker and the dynamic loader and even adds slight runtime overhead. Primarily, these functions are just simple one-time helper functions but we will have to apply general API stabilizing rules to them (like restrict changing behavior of a function without renaming it or so). Although, this is not going to be completely killing us because we do not provide API backward-compatibility across major releases. Besides that, the bloated API can confuse new users of ns-3. > 2) move the static function to an anonymous namespace. again, no warnings with > any compiler I am aware of. Intel C++ Compiler (e.g., 11.1) produces the following warning: 'remark #177: function "<unnamed>::qqq" was declared but never referenced' icpc -x c++ - -Wall -o /dev/null <<EOFF namespace { int qqq () { return 0; } } int main () { return 0; } EOFF So there is no guarantee, that, GCC will not warn in the same case in the near future.
(In reply to comment #18) > > 4) some code needs to execute in an optimized environment, but the > > return value is only used in a debug environment. Wrap the declaration > > as follows: > > nack. introduce a helper function for the debug code, do as usual for the > optimized code. > > > > > NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); > > which expands to (in debug build): > > uint8_t tmp = start.ReadU8 (); > > or (in optimized build): > > start.ReadU8 (); > > > > A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping > > assert-related temporary variables. I'm sorry, but I can't come up with a smart helper function for the case. Could you, please, give an example of such a function and it's usage?
I just had to deal with this issue again in changeset 2778027c929c. This is a bump to try to resolve and close this issue. To review where we seem to be, Andrey proposed a set of new macros to cover the various issues, Mathieu proposed instead to handle some of these with helper functions, and Andrey raised a few concerns with the use of helper functions (API bloat and bigger symbol table). I would prefer the macros partly for the reasons Andrey cited, and partly because I think it will be more cumbersome for the model developers to have to declare and define these functions for (typically) one-time uses.
(In reply to comment #21) > I just had to deal with this issue again in changeset 2778027c929c. > > This is a bump to try to resolve and close this issue. To review where we seem > to be, Andrey proposed a set of new macros to cover the various issues, Mathieu > proposed instead to handle some of these with helper functions, and Andrey > raised a few concerns with the use of helper functions (API bloat and bigger > symbol table). > > I would prefer the macros partly for the reasons Andrey cited, and partly > because I think it will be more cumbersome for the model developers to have to > declare and define these functions for (typically) one-time uses. To be honest I can't see why we can't use both approaches. My own rule of thumb is: define a helper function if you're going to use it more than once, otherwise don't use one. Usually this means that, if I have to put a lot of debugging stuff, I'll end up with an helper function like Mathieu suggested. Otherwise I'd use macros. It's a matter of flexibility. Let's keep in mind that rules are there to be broken and guidelines are there to be ignored. Maybe I'm a little harsh, but I use the guidelines (like the manuals) to be able to slap people asking dumb questions, and it happens too frequently. My own suggestion is: have the guidelines outlining both approaches, as they're both valid. Guidelines should allow each coder to choose what they think it's best. If we make them too "hard" they'll simply not follow them. And we'll have to slap them twice a day. Sure, it can be fun, but after a while it's boring. Cheers, T.
BTW... Sorry Tom, that patch was mine :P /hide in shame T.
(In reply to comment #19) > (In reply to comment #17) > > > > Ipv4Address ToIpv4 (const Address &addr) > > > > { > > > > InetSocketAddress iaddr = InetSocketAddress::ConvertFrom (addr); > > > > return iaddr.GetIpv4 (); > > > > } > > > > > > > > Same for ToPort > > > > > > > > and finally: > > > > NS_LOG_DEBUG ("Received one packet! Socket: " << ToIpv4(addr) << " port: " << > > > > ToPort (addr)); > > > > > > > This is indeed a good point and it is true in C (although, really, from a > > simplicity point of view, I would be willing to live with non-static functions > > for that in C), but this is C++ so, there are a couple of classic tricks around > > this issue: > > > > 1) make the static function a member static method: no warnings anymore, even > > with recent gccs. > > Considering the above ToIpv4() example, which class should we make the static > method member of? > I see two options: > 1.1) Make ToIpv4() method a member of Ipv4Address class > 1.2) Make ToIpv4() method a member of the class that uses it (like PacketSink > in this example) > > Option 1.1) requires someone to modify the core class whenever he needs a new > NS_LOG_*()/NS_ASSER() statement. > Also this will be impossible when the app store idea gets implemented. > Addition of helper functions to the core class will also introduce additional > dependencies and will end up with everything depending on everything. > > Both options make the whole ns-3 API bloated with a bunch of (duplicated) > helper functions, occupying more space in dynamic symbol table, which adds more > work to both the linker and the dynamic loader and even adds slight runtime > overhead. > Primarily, these functions are just simple one-time helper functions but we > will have to apply general API stabilizing rules to them (like restrict > changing behavior of a function without renaming it or so). > Although, this is not going to be completely killing us because we do not > provide API backward-compatibility across major releases. > Besides that, the bloated API can confuse new users of ns-3. 1) I would never expect anyone to try to share the code of this two-line function. i.e., copy/paste has its uses. 2) If this function was deemed really useful, then, yes, we should add it as a member method of the relevant class or as some utils function. We have an ipv4-utils.h somewhere, I believe ? > > > 2) move the static function to an anonymous namespace. again, no warnings with > > any compiler I am aware of. > Intel C++ Compiler (e.g., 11.1) produces the following warning: > 'remark #177: function "<unnamed>::qqq" was declared but never referenced' > > icpc -x c++ - -Wall -o /dev/null <<EOFF > namespace { > int qqq () > { > return 0; > } > } > int main () > { > return 0; > } > EOFF > > So there is no guarantee, that, GCC will not warn in the same case in the near > future. Sure. Use a non-static global function. As I said before, this is a common trick in C.
(In reply to comment #20) > (In reply to comment #18) > > > 4) some code needs to execute in an optimized environment, but the > > > return value is only used in a debug environment. Wrap the declaration > > > as follows: > > > > nack. introduce a helper function for the debug code, do as usual for the > > optimized code. > > > > > > > > NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); > > > which expands to (in debug build): > > > uint8_t tmp = start.ReadU8 (); > > > or (in optimized build): > > > start.ReadU8 (); > > > > > > A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping > > > assert-related temporary variables. > > I'm sorry, but I can't come up with a smart helper function for the case. > Could you, please, give an example of such a function and it's usage? No, not in a general way but if you could show an actual example where you have a problem, I can point out how to refactor the code. In general, my advice is to be willing to make the debugging code do redundant work. i.e., the normal codepath does something and the debug codepath will do the same thing a second time: there is no sharing of variables between the two.
(In reply to comment #21) > I just had to deal with this issue again in changeset 2778027c929c. > > This is a bump to try to resolve and close this issue. To review where we seem > to be, Andrey proposed a set of new macros to cover the various issues, Mathieu > proposed instead to handle some of these with helper functions, and Andrey > raised a few concerns with the use of helper functions (API bloat and bigger > symbol table). > > I would prefer the macros partly for the reasons Andrey cited, and partly > because I think it will be more cumbersome for the model developers to have to > declare and define these functions for (typically) one-time uses. The obvious way to avoid the ifdef would be to use an explicit if statement to wrap NS_ASSERT_MSG (the compiler will optimize away the empty statement), but, really, NS_ASSERT_MSG should be doing this for me instead. In general, though, I believe that the debugging macros are just plain wrong because they do not generate the code for the if statement unconditionally. They should and then rely on the compiler to optimize stuff away. This is what they are here for. i.e., a patch to replace: #else /* NS3_ASSERT_ENABLE */ #define NS_ASSERT(cond) with: #else /* NS3_ASSERT_ENABLE */ #define NS_ASSERT(condition) \ do \ { \ if (!(condition)) \ { \ } \ } \ while (false) is pre-approved (same for log.h) I am actually surprised that the code we have is like this because this is a common thing for log/assert macros to do.
(In reply to comment #22) > Usually this means that, if I have to put a lot of debugging stuff, I'll end up > with an helper function like Mathieu suggested. Otherwise I'd use macros. It's > a matter of flexibility. I simply do not see these macros as sufficiently widely useful that they deserve new API.
(In reply to comment #26) > The obvious way to avoid the ifdef would be to use an explicit if statement to > wrap NS_ASSERT_MSG (the compiler will optimize away the empty statement), but, > really, NS_ASSERT_MSG should be doing this for me instead. > > In general, though, I believe that the debugging macros are just plain wrong > because they do not generate the code for the if statement unconditionally. > They should and then rely on the compiler to optimize stuff away. This is what > they are here for. > > i.e., a patch to replace: > > #else /* NS3_ASSERT_ENABLE */ > #define NS_ASSERT(cond) > > with: > #else /* NS3_ASSERT_ENABLE */ > #define NS_ASSERT(condition) \ > do \ > { \ > if (!(condition)) \ > { \ > } \ > } \ > while (false) > > is pre-approved (same for log.h) > > I am actually surprised that the code we have is like this because this is a > common thing for log/assert macros to do. While this is a common trick, the compiler will produce code to evaluate the condition if it contains any function calls. And this contradicts to the statement that no debug code should execute in optimized build.
(In reply to comment #24) > > > 2) move the static function to an anonymous namespace. again, no warnings with > > > any compiler I am aware of. > > Intel C++ Compiler (e.g., 11.1) produces the following warning: > > 'remark #177: function "<unnamed>::qqq" was declared but never referenced' > > > > icpc -x c++ - -Wall -o /dev/null <<EOFF > > namespace { > > int qqq () > > { > > return 0; > > } > > } > > int main () > > { > > return 0; > > } > > EOFF > > > > So there is no guarantee, that, GCC will not warn in the same case in the near > > future. > > > Sure. Use a non-static global function. As I said before, this is a common > trick in C. Besides export of unneeded functions and need to prevent name collisions within a module, this is ok.
(In reply to comment #25) > (In reply to comment #20) > > (In reply to comment #18) > > > > 4) some code needs to execute in an optimized environment, but the > > > > return value is only used in a debug environment. Wrap the declaration > > > > as follows: > > > > > > nack. introduce a helper function for the debug code, do as usual for the > > > optimized code. > > > > > > > > > > > NS_LOG_ASSIGNMENT(uint8_t tmp, start.ReadU8 ()); > > > > which expands to (in debug build): > > > > uint8_t tmp = start.ReadU8 (); > > > > or (in optimized build): > > > > start.ReadU8 (); > > > > > > > > A similar macro NS_ASSERT_ASSIGMENT() should be used for wrapping > > > > assert-related temporary variables. > > > > I'm sorry, but I can't come up with a smart helper function for the case. > > Could you, please, give an example of such a function and it's usage? > > No, not in a general way but if you could show an actual example where you have > a problem, I can point out how to refactor the code. In general, my advice is > to be willing to make the debugging code do redundant work. i.e., the normal > codepath does something and the debug codepath will do the same thing a second > time: there is no sharing of variables between the two. Oh, now I better understand your point. I'll try to create an updated patch leant to your approach and ask here for help if I find a hard function to refactor this time.
(In reply to comment #28) > (In reply to comment #26) > > The obvious way to avoid the ifdef would be to use an explicit if statement to > > wrap NS_ASSERT_MSG (the compiler will optimize away the empty statement), but, > > really, NS_ASSERT_MSG should be doing this for me instead. > > > > In general, though, I believe that the debugging macros are just plain wrong > > because they do not generate the code for the if statement unconditionally. > > They should and then rely on the compiler to optimize stuff away. This is what > > they are here for. > > > > i.e., a patch to replace: > > > > #else /* NS3_ASSERT_ENABLE */ > > #define NS_ASSERT(cond) > > > > with: > > #else /* NS3_ASSERT_ENABLE */ > > #define NS_ASSERT(condition) \ > > do \ > > { \ > > if (!(condition)) \ > > { \ > > } \ > > } \ > > while (false) > > > > is pre-approved (same for log.h) > > > > I am actually surprised that the code we have is like this because this is a > > common thing for log/assert macros to do. > > While this is a common trick, the compiler will produce code to evaluate the > condition if it contains any function calls. > And this contradicts to the statement that no debug code should execute in > optimized build. This is a great point. It took a bit of googling to finally come up with what appears to be a common way to do this: http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/ (I did discuss this specific hack on the gcc-help mailing-list where others suggested the same approach) i.e., (void)(sizeof(statement)); patches welcome.
(In reply to comment #31) > > While this is a common trick, the compiler will produce code to evaluate the > > condition if it contains any function calls. > > And this contradicts to the statement that no debug code should execute in > > optimized build. > > This is a great point. It took a bit of googling to finally come up with what > appears to be a common way to do this: > http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/ > > (I did discuss this specific hack on the gcc-help mailing-list where others > suggested the same approach) > > i.e., > > (void)(sizeof(statement)); > > patches welcome. Wow, it's a very nice trick I haven't seen before. Unfortunately, Intel C++ Compiler gives "remark #593: variable "b" was set but never used" for the code: """ #define MyAssert(x) do { (void)sizeof(x); } while (0) int DoJob (int a); void qqq (int a) { int b = DoJob (a); MyAssert (b); } """ But it's not a supported platform, so we can ignore it. Anyway this assert implementation is better than that we have now.
(In reply to comment #32) > > This is a great point. It took a bit of googling to finally come up with what > > appears to be a common way to do this: > > http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/ > > > > (I did discuss this specific hack on the gcc-help mailing-list where others > > suggested the same approach) > > > > i.e., > > > > (void)(sizeof(statement)); > > > > patches welcome. > Wow, it's a very nice trick I haven't seen before. > > Unfortunately, Intel C++ Compiler gives > "remark #593: variable "b" was set but never used" > for the code: > """ > #define MyAssert(x) do { (void)sizeof(x); } while (0) > > int DoJob (int a); > > void qqq (int a) > { > int b = DoJob (a); > MyAssert (b); > } > """ > But it's not a supported platform, so we can ignore it. > > Anyway this assert implementation is better than that we have now. Hmm. the downside of this implementation is that it silences compiler even when it shouldn't. For example, (from src/internet/test/ipv4-raw-test.cc) """ void Ipv4RawSocketImplTest::ReceivePkt (Ptr<Socket> socket) { uint32_t availableData; availableData = socket->GetRxAvailable (); m_receivedPacket = socket->Recv (2, MSG_PEEK); NS_ASSERT (m_receivedPacket->GetSize () == 2); m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); NS_ASSERT (availableData == m_receivedPacket->GetSize ()); } """ This will successfully compile (with NS_ASSERT() implemented as sizeof()) and emit the call to GetRxAvailable(). But the call to GetRxAvailable() is needed only for debug build. The only solution is to mark GetRxAvailable() with attribute pure or const, but I suppose, that it's again gcc-specific. So in this particular case, I'd prefer the old NS_ASSERT() implementation which will trigger "set but not used" warning.
(In reply to comment #33) > (In reply to comment #32) > > > This is a great point. It took a bit of googling to finally come up with what > > > appears to be a common way to do this: > > > http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/ > > > > > > (I did discuss this specific hack on the gcc-help mailing-list where others > > > suggested the same approach) > > > > > > i.e., > > > > > > (void)(sizeof(statement)); > > > > > > patches welcome. > > Wow, it's a very nice trick I haven't seen before. > > > > Unfortunately, Intel C++ Compiler gives > > "remark #593: variable "b" was set but never used" > > for the code: > > """ > > #define MyAssert(x) do { (void)sizeof(x); } while (0) > > > > int DoJob (int a); > > > > void qqq (int a) > > { > > int b = DoJob (a); > > MyAssert (b); > > } > > """ > > But it's not a supported platform, so we can ignore it. More precisely, we can ignore warnings. i.e., We do not care if the icc builds do not compile with -Werror. > > > > Anyway this assert implementation is better than that we have now. > > Hmm. the downside of this implementation is that it silences compiler even when > it shouldn't. > For example, (from src/internet/test/ipv4-raw-test.cc) > """ > void Ipv4RawSocketImplTest::ReceivePkt (Ptr<Socket> socket) > { > uint32_t availableData; > availableData = socket->GetRxAvailable (); > m_receivedPacket = socket->Recv (2, MSG_PEEK); > NS_ASSERT (m_receivedPacket->GetSize () == 2); > m_receivedPacket = socket->Recv (std::numeric_limits<uint32_t>::max (), 0); > NS_ASSERT (availableData == m_receivedPacket->GetSize ()); > } > """ > This will successfully compile (with NS_ASSERT() implemented as sizeof()) and > emit the call to GetRxAvailable(). > But the call to GetRxAvailable() is needed only for debug build. It is not needed for optimized builds but: 1) the code is unambiguous about what it is doing: an unconditional call to GetRxAvailable 2) if you care about optimizing away that function call, you must/can do the work needed to optimize it: either use pure const or #ifdef magic. > So in this particular case, I'd prefer the old NS_ASSERT() implementation which > will trigger "set but not used" warning. In this particular case, the user who writes the code is free to do what he wants (ifdef or gcc-spsecific attribute magic, I could not care less) but at least, the newer NS_ASSERT implementation makes sure that we do not get a failing opitmized build for no decent reason. Yes, it fails to warn the user that he has a choice. I can live with that.
Created attachment 1329 [details] updated patch trying to address Mathieu's comments This version of patch doesn't add NS_ASSERT_ASSIGNMENT() and friends macros, but does introduce NS_UNUSED() macro for rare uses. It also adds new implementation of NS_ASSERT() discussed here. Several debug-only functions are added with their body enclosed in #ifdef NS3_ASSERT_ENABLE/#endif block. But I'd prefer declare them static and enclose the whole function in such a block.
Created attachment 1331 [details] The same patch rebased against 7703:a982d8c7aa32
(In reply to comment #36) > Created attachment 1331 [details] > The same patch rebased against 7703:a982d8c7aa32 It looks like it is not possible to comment on the patch itself in bugzilla. Damn. 1) overall, the patch looks good to me: I am fine with NS_UNUSED, the improvement to NS_ASSERT/MSG is great. 2) I have comments on some of the logging calls. Here are a couple. Feel free to commit the patch as-is first and leave these improvements as a second commit if you like them. Most of my comments are really about the fact that it is better to make Print functions return a string and call them from the logging macros. Some of my comments are really about removing aggressively ifdefs if possible to avoid having to maintain code that contains them and to avoid giving our newer contributors bad ideas about the use of ifdefs. Anyway, thanks for this patch: it is a great improvement over what we have. A. I would move the logging call to the relevant callback and get rid of the #ifdef. https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/examples/energy/energy-model-example.cc_sec2 B. I would change the signature of PrintReceivedPacket to return a string generated with std::ostringstream and then call NS_LOG_UNCOND(PrintReceivedPacket()) from ReceivePacket and then remove the #ifdef in PrintReceivedPacket. https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/examples/routing/manet-routing-compare.cc_sec2 C. Same as B https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/examples/wireless/multirate.cc_sec4 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/applications/model/packet-sink.cc_sec1 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/examples/wireless/wifi-simple-interference.cc_sec1 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/test/ns3wifi/wifi-interference-test-suite.cc_sec1 D. I have had a good laugh there. https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/core/examples/main-callback.cc_sec1 E. Can't you remove the ifdefs from here ? (These are tests, no cares a single bit if they call a couple of extra functions unconditionally in optimized builds) https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/internet/test/ipv4-header-test.cc_sec1 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/internet/test/ipv4-raw-test.cc_sec1 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/internet/test/udp-test.cc_sec1 https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/internet/test/udp-test.cc_sec2 F. I really liked the previous version of the code better: without an ifdef: https://www.nsnam.org/bugzilla/attachment.cgi?id=1331&action=diff#a/src/network/utils/radiotap-header.cc_sec1
Created attachment 1332 [details] Updated (2nd time) patch to address Mathieu's comments. Mathieu, I mostly agree with your comments and have tried to address them.
(In reply to comment #38) > Created attachment 1332 [details] > Updated (2nd time) patch to address Mathieu's comments. > > Mathieu, I mostly agree with your comments and have tried to address them. Oops, I guess, I've missed src/topology-read/model/rocketfuel-topology-reader.cc while making Print* functions return a string. I'll fix this later today.
(In reply to comment #39) > (In reply to comment #38) > > Created attachment 1332 [details] > > Updated (2nd time) patch to address Mathieu's comments. > > > > Mathieu, I mostly agree with your comments and have tried to address them. > > Oops, I guess, I've missed > src/topology-read/model/rocketfuel-topology-reader.cc while making Print* > functions return a string. > I'll fix this later today. I've taken a look into this and have decided to leave that piece of code as in the latest patch (making PrintNodeInfo () return a string triggers compiler warnings). Mathieu supports merging this patch (he told me that he could not comment on the bugzilla on Friday for some reason). So unless there are any other objections, I'll push this patch today evening.
(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > > Created attachment 1332 [details] > > > Updated (2nd time) patch to address Mathieu's comments. > > > > > > Mathieu, I mostly agree with your comments and have tried to address them. > > > > Oops, I guess, I've missed > > src/topology-read/model/rocketfuel-topology-reader.cc while making Print* > > functions return a string. > > I'll fix this later today. > > I've taken a look into this and have decided to leave that piece of code as in > the latest patch (making PrintNodeInfo () return a string triggers compiler > warnings). > > Mathieu supports merging this patch (he told me that he could not comment on > the bugzilla on Friday for some reason). > > So unless there are any other objections, I'll push this patch today evening. for the record, +1 for merging.
(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > > Created attachment 1332 [details] > > > Updated (2nd time) patch to address Mathieu's comments. > > > > > > Mathieu, I mostly agree with your comments and have tried to address them. > > > > Oops, I guess, I've missed > > src/topology-read/model/rocketfuel-topology-reader.cc while making Print* > > functions return a string. > > I'll fix this later today. > > I've taken a look into this and have decided to leave that piece of code as in > the latest patch (making PrintNodeInfo () return a string triggers compiler > warnings). > > Mathieu supports merging this patch (he told me that he could not comment on > the bugzilla on Friday for some reason). > > So unless there are any other objections, I'll push this patch today evening. +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).
(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.
(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).
> > 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. Looks good to me, and if no other comments, I could update the wordpress page accordingly and close this.
Created attachment 1501 [details] patch to make Print*() functions static inline Small update on this old issue. Mathieu's suggestion was to introduce non-static global functions to avoid compiler warnings about unused functions. I've just figured out that we can use static inline functions instead. Compilers don't warn on unused static inline functions too, but instead can optimized them out completely and never put them into a symbol table.
(In reply to comment #46) > Created attachment 1501 [details] > patch to make Print*() functions static inline > > Small update on this old issue. > > Mathieu's suggestion was to introduce non-static global functions to avoid > compiler warnings about unused functions. > > I've just figured out that we can use static inline functions instead. > Compilers don't warn on unused static inline functions too, but instead can > optimized them out completely and never put them into a symbol table. nice trick. +1
(In reply to comment #47) > (In reply to comment #46) > > Created attachment 1501 [details] > > patch to make Print*() functions static inline > > > > Small update on this old issue. > > > > Mathieu's suggestion was to introduce non-static global functions to avoid > > compiler warnings about unused functions. > > > > I've just figured out that we can use static inline functions instead. > > Compilers don't warn on unused static inline functions too, but instead can > > optimized them out completely and never put them into a symbol table. > > nice trick. +1 Thank you! If there are no objections, I'll push the patch on Friday.
(In reply to comment #48) > (In reply to comment #47) [...] > > nice trick. +1 > > Thank you! > If there are no objections, I'll push the patch on Friday. The updated proposed 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 static inline 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 static inline 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 static inline 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 static inline 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).
Created attachment 1502 [details] updated patch to make Print*() functions static inline Added one more function converted to static inline.
(In reply to comment #50) > Created attachment 1502 [details] > updated patch to make Print*() functions static inline > > Added one more function converted to static inline. I've pushed the patch attachment 1502 [details] as changeset cca3ebe36928.
> > The updated proposed section to > http://www.nsnam.org/developers/contributing-code/coding-style/: > Andrey, what is the recommended way to deal with variables used to check return values of methods that must execute even in optimized code? (this case is left out of the proposed coding style amendment) e.g. bool ok = m_object->SetAttributeFailSafe ("MyIntegerAttribute", IntegerValue (25))); NS_ASSERT (ok == true);
(In reply to comment #52) > > > > The updated proposed section to > > http://www.nsnam.org/developers/contributing-code/coding-style/: > > > > Andrey, what is the recommended way to deal with variables used to check > return values of methods that must execute even in optimized code? (this > case is left out of the proposed coding style amendment) > > e.g. > > bool ok = m_object->SetAttributeFailSafe ("MyIntegerAttribute", > IntegerValue (25))); > NS_ASSERT (ok == true); For NS_ASSERT() the above code should work as is. For NS_LOG() it should be refactored as described in case 3), like: static void PrintSetAttributeResult (Ptr<Object> object, bool result) { NS_LOG_DEBUG("SetAttribute result on object " object << " is " << result); } ... bool ok = m_object->SetAttributeFailSafe ("MyIntegerAttribute", IntegerValue (25))); PrintSetAttributeResult (m_object, ok);
To clarify, then, once the coding style is updated based on comment 44, this bug may be closed, correct?
(In reply to comment #54) > To clarify, then, once the coding style is updated based on comment 44, this > bug may be closed, correct? I think, yes, the only thing left is the coding style update. But I suppose, it should be updated based on comment 49 as it's the most up-to-date proposal.
I upgraded our OS X buildslave to Xcode 5.1 yesterday, and it triggered a build error related to how we are handling these. ../examples/wireless/multirate.cc:322:1: error: unused function 'PrintPosition' [-Werror,-Wunused-function] PrintPosition (Ptr<Node> client, Ptr<Node> server) ^ 1 error generated. the compiler version is: Apple LLVM version 5.1 (clang-503.0.38) (based on LLVM 3.4svn) Target: x86_64-apple-darwin13.1.0 Thread model: posix The technique that no longer works is: 2) complex debug-only code around NS_ASSERT() and NS_LOG() should be factored out into a separate static inline global function. which is what "PrintPosition()" was refactored into when this bug was worked on. I haven't yet found anything in the release notes corresponding to this changed behavior; will look further.
My Mac (same compiler version) is not throwing this error. Needs further investigations. T.
(In reply to Tommaso Pecorella from comment #57) > My Mac (same compiler version) is not throwing this error. > > Needs further investigations. Just in case: are you building an optimized version of ns3 (I mean `./waf configure -d optimized`)?
(In reply to Andrey Mazo from comment #58) > (In reply to Tommaso Pecorella from comment #57) > > My Mac (same compiler version) is not throwing this error. > > > > Needs further investigations. > > Just in case: are you building an optimized version of ns3 (I mean `./waf > configure -d optimized`)? Please hit me with something heavy.
(In reply to Tommaso Pecorella from comment #59) > (In reply to Andrey Mazo from comment #58) > > (In reply to Tommaso Pecorella from comment #57) > > > My Mac (same compiler version) is not throwing this error. > > > > > > Needs further investigations. > > > > Just in case: are you building an optimized version of ns3 (I mean `./waf > > configure -d optimized`)? > > Please hit me with something heavy. I think the problem is a bit more complex that expected. "inline" is an hint, not an order. In this case it seems that the compiler isn't inlining the function, thus the error. It seems that we need a more definitive solution...
http://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Variable-Attributes.html i.e.: static std::string PrintReceivedPacket (Ptr<Socket> socket) __attribute__ ((unused)); std::string PrintReceivedPacket (Ptr<Socket> socket) The inline is not even needed. I'm sure it works with clang (it's supported) but I'm not 100% sure about wider compatibility. In particular I'm not aware of how VisualC++ would react to this. One solution may be to use both. Inlining *and* the attribute. Is this a viable solution ?
(In reply to Andrey Mazo from comment #58) > (In reply to Tommaso Pecorella from comment #57) > > My Mac (same compiler version) is not throwing this error. > > > > Needs further investigations. > > Just in case: are you building an optimized version of ns3 (I mean `./waf > configure -d optimized`)? Yes, sorry I didn't mention it. Here is the failure: https://ns-buildmaster.ee.washington.edu:8010/job/osx-mlion-ns-3-allinone/label=master/432/console
(In reply to Tommaso Pecorella from comment #61) > http://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Variable-Attributes.html > > i.e.: > static std::string PrintReceivedPacket (Ptr<Socket> socket) __attribute__ > ((unused)); > std::string PrintReceivedPacket (Ptr<Socket> socket) > > The inline is not even needed. > > I'm sure it works with clang (it's supported) but I'm not 100% sure about > wider compatibility. In particular I'm not aware of how VisualC++ would > react to this. > > One solution may be to use both. Inlining *and* the attribute. > > Is this a viable solution ? As far as I remember, the original discussion came to the agreement, that we should avoid using all this compiler-specific __attribute__ stuff. Otherwise, we might use __attribute__((unused)) directly on several variables in the first place. I see two solutions now: 1) come back to Mathieu's suggestion to use global non-static C functions; 2) move these static inline functions (like Print*()) to separate headers -- it seems to silence clang for now.
(In reply to Andrey Mazo from comment #63) > I see two solutions now: > 1) come back to Mathieu's suggestion to use global non-static C functions; > 2) move these static inline functions (like Print*()) to separate headers -- > it seems to silence clang for now. Forgot to mention, that I would go for solution 1), because moving these functions to separate files makes the code harder to read and follow.
How about instead keeping our current practice and just disabling this warning for optimized builds? diff -r 67ec151083bd wscript --- a/wscript Fri Mar 21 16:57:41 2014 +0100 +++ b/wscript Sun Mar 23 07:40:03 2014 -0700 @@ -325,6 +325,7 @@ if Options.options.build_profile == 'optimized': if conf.check_compilation_flag('-march=native'): env.append_value('CXXFLAGS', '-march=native') + env.append_value('CXXFLAGS', '-Wno-unused-function') if sys.platform == 'win32': env.append_value("LINKFLAGS", "-Wl,--enable-runtime-pseudo-reloc") Any drawbacks with this solution?
None I could think of. +1 PS: what about disabling also the unused variables warning in optimised builds ? We could forget about the NS_UNUSED... T.
(In reply to Tom Henderson from comment #65) > How about instead keeping our current practice and just disabling this > warning for optimized builds? > > diff -r 67ec151083bd wscript > --- a/wscript Fri Mar 21 16:57:41 2014 +0100 > +++ b/wscript Sun Mar 23 07:40:03 2014 -0700 > @@ -325,6 +325,7 @@ > if Options.options.build_profile == 'optimized': > if conf.check_compilation_flag('-march=native'): > env.append_value('CXXFLAGS', '-march=native') > + env.append_value('CXXFLAGS', '-Wno-unused-function') > > if sys.platform == 'win32': > env.append_value("LINKFLAGS", > "-Wl,--enable-runtime-pseudo-reloc") > > Any drawbacks with this solution? I'm afraid, we can miss really unused functions in complex templates/classes mix then.
Created attachment 1807 [details] fixed clang-3.4 warnings I've found one more solution. I've modified NS_LOG* macros to be "if (false) { clog << msg; }" instead of just nothing. As this trick is used only in optimized version, I'm sure all compilers are able to remove this kind of dead code. This makes clang silent again (gcc is fine too). Most of NS_LOG* call sites need no modification, others are fixed in the patch. The only thing I'm afraid of is that future versions of clang or gcc may detect and warn on dead code. I haven't tested these modules yet: brite click openflow visualizer
I'm fine with all the options. The __attribute__ one is pseudo-standard. I know it's working on most compilers, but I agree that it's not 100% standard. The -Wno-unused-function option is fine as well for me. It's not real hurting imho, as dead functions should be signalled anyway in debug builds. The NS_LOG workaround seems the less intrusive, restoring the actual behaviour. What I don't really like is the fact that, sooner or later, the compiler will get smarter and will start to complain (again). What I can only hope for is: to have a standard version of the __attribute__((unused)). T
(In reply to Andrey Mazo from comment #67) > (In reply to Tom Henderson from comment #65) > > How about instead keeping our current practice and just disabling this > > warning for optimized builds? > > > > diff -r 67ec151083bd wscript > > --- a/wscript Fri Mar 21 16:57:41 2014 +0100 > > +++ b/wscript Sun Mar 23 07:40:03 2014 -0700 > > @@ -325,6 +325,7 @@ > > if Options.options.build_profile == 'optimized': > > if conf.check_compilation_flag('-march=native'): > > env.append_value('CXXFLAGS', '-march=native') > > + env.append_value('CXXFLAGS', '-Wno-unused-function') > > > > if sys.platform == 'win32': > > env.append_value("LINKFLAGS", > > "-Wl,--enable-runtime-pseudo-reloc") > > > > Any drawbacks with this solution? > > I'm afraid, we can miss really unused functions in complex templates/classes > mix then. Won't they be caught in debug builds? Anyway, I'm fine with proceeding with your patch to fix as well.
(In reply to Tom Henderson from comment #70) > (In reply to Andrey Mazo from comment #67) [...] > > I'm afraid, we can miss really unused functions in complex templates/classes > > mix then. > > Won't they be caught in debug builds? Yes, they most likely will. I originally thought that a compiler could produce more warnings while performing optimizations than while not performing them. But I can't find any authoritative prove of it. [1] So, now, I mostly fine with -Wno-unused-function for optimized builds. And we can even add it only for clang. > Anyway, I'm fine with proceeding with your patch to fix as well. Thank you! [1] Influence of optimization level on amount of warnings is explicitly noted for -Wuninitialized in gcc man-page, but nothing similar is said about -Wunused-function.
Andrey, which way do you want to go with this patch? - your patch above - disabling the warning as I suggested - disabling the warning in a clang-specific way I don't have a strong preference, but we should just decide one and patch it. We should also add a note to the errata about a workaround in case someone encounters this by trying to build ns-3.19 optimized with this compiler: http://www.nsnam.org/wiki/Ns-3.19-errata
(In reply to Tom Henderson from comment #72) > Andrey, which way do you want to go with this patch? > > - your patch above > - disabling the warning as I suggested > - disabling the warning in a clang-specific way > > I don't have a strong preference, but we should just decide one and patch it. > > We should also add a note to the errata about a workaround in case someone > encounters this by trying to build ns-3.19 optimized with this compiler: > http://www.nsnam.org/wiki/Ns-3.19-errata I'm kind of an interested party, so my choice may not be independent.:) But I would go with my patch. And in case of future troubles (like compilers getting more intelligent), we'll be able to revert the patch and just disable the warnings (or disable errors on the warnings with -Wno-error=unused-function).
Should I proceed with pushing my (or any other) solution?
(In reply to Andrey Mazo from comment #74) > Should I proceed with pushing my (or any other) solution? Please push your patch; there were no other comments.
(In reply to Tom Henderson from comment #75) > (In reply to Andrey Mazo from comment #74) > > Should I proceed with pushing my (or any other) solution? > > Please push your patch; there were no other comments. Pushed as changeset 1330d4ee94e8. Marking the bug as resolved (again :)).
(In reply to Andrey Mazo from comment #76) > (In reply to Tom Henderson from comment #75) > > (In reply to Andrey Mazo from comment #74) > > > Should I proceed with pushing my (or any other) solution? > > > > Please push your patch; there were no other comments. > > Pushed as changeset 1330d4ee94e8. > Marking the bug as resolved (again :)). I've updated the errata page [1] too. [1] http://www.nsnam.org/wiki/Ns-3.19-errata