|
|
Created:
6 years, 4 months ago by ammo6818-vandals.uidaho.edu Modified:
6 years ago Reviewers:
Stefano Avallone CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
DescriptionEliminate Visual Studio complier warnings
Patch Set 1 #Patch Set 2 : Updates to use C++ style type casts #
Total comments: 16
Patch Set 3 : Updated patch set to address review comments #
Total comments: 32
Patch Set 4 : Updates to patch for review comments #Patch Set 5 : Patch updates for x64 build #Patch Set 6 : Final change required to eliminate warnings #MessagesTotal messages: 12
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... File src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc:235: float startTime = 0.1f; OK. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... src/traffic-control/examples/codel-vs-pfifo-basic-test.cc:90: float startTime = 0.1f; OK. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... File src/traffic-control/examples/pfifo-vs-red.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... src/traffic-control/examples/pfifo-vs-red.cc:174: totalRxBytesCounter += static_cast<uint32_t>(pktSink->GetTotalRx ()); I declared totalRxBytesCounter as uint64_t. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helpe... File src/traffic-control/helper/traffic-control-helper.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helpe... src/traffic-control/helper/traffic-control-helper.cc:88: for (uint8_t i = 0; i < m_queueDiscClassesFactory.size (); i++) This should be uint16_t. Does the VS compiler output a warning with uint16_t? https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... File src/traffic-control/model/fq-codel-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... src/traffic-control/model/fq-codel-queue-disc.cc:282: flow->IncreaseDeficit (-static_cast<int32_t>(item->GetSize ())); Is: flow->IncreaseDeficit (item->GetSize () * -1); ok? https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... File src/traffic-control/model/traffic-control-layer.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... src/traffic-control/model/traffic-control-layer.cc:109: for (uint8_t i = 0; i < devQueueIface->GetNTxQueues (); i++) OK. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... src/traffic-control/model/traffic-control-layer.cc:119: for (uint8_t i = 0; i < devQueueIface->GetNTxQueues (); i++) OK. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:201: queue->Enqueue (Create<CodelQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0))); Removed the third parameter from the constructor. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/pie-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/pie-queue-disc-test-suite.cc:183: queue->Enqueue (Create<PieQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0))); Removed the third parameter from the constructor. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/red-queue-disc-test-suite.cc:174: queue->Enqueue (Create<RedQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0), false)); Removed the third parameter from the constructor.
Sign in to reply to this message.
Updated patch set has been uploaded to address your review comments. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... File src/traffic-control/examples/pfifo-vs-red.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examp... src/traffic-control/examples/pfifo-vs-red.cc:174: totalRxBytesCounter += static_cast<uint32_t>(pktSink->GetTotalRx ()); On 2017/12/28 23:55:27, Stefano Avallone wrote: > I declared totalRxBytesCounter as uint64_t. incorporated change. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helpe... File src/traffic-control/helper/traffic-control-helper.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helpe... src/traffic-control/helper/traffic-control-helper.cc:88: for (uint8_t i = 0; i < m_queueDiscClassesFactory.size (); i++) On 2017/12/28 23:55:27, Stefano Avallone wrote: > This should be uint16_t. Does the VS compiler output a warning with uint16_t? changed to uint16_t https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... File src/traffic-control/model/fq-codel-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model... src/traffic-control/model/fq-codel-queue-disc.cc:282: flow->IncreaseDeficit (-static_cast<int32_t>(item->GetSize ())); On 2017/12/28 23:55:27, Stefano Avallone wrote: > Is: > > flow->IncreaseDeficit (item->GetSize () * -1) affirmative. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:201: queue->Enqueue (Create<CodelQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0))); On 2017/12/28 23:55:27, Stefano Avallone wrote: > Removed the third parameter from the constructor. change applied https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/pie-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/pie-queue-disc-test-suite.cc:183: queue->Enqueue (Create<PieQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0))); On 2017/12/28 23:55:27, Stefano Avallone wrote: > Removed the third parameter from the constructor. change incorporated. https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... File src/traffic-control/test/red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/... src/traffic-control/test/red-queue-disc-test-suite.cc:174: queue->Enqueue (Create<RedQueueDiscTestItem> (p1, dest, static_cast<uint16_t>(0), false)); On 2017/12/28 23:55:27, Stefano Avallone wrote: > Removed the third parameter from the constructor. change incorporated.
Sign in to reply to this message.
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/adaptive-red-tests.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/adaptive-red-tests.cc:180: #if 0 Why? https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/adaptive-red-tests.cc:206: Why? https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/codel-vs-pfifo-basic-test.cc:91: float simDuration = 60; //in seconds please do not remove the white space, the comment is already aligned. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/pie-example.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/pie-example.cc:162: Why? https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/red-tests.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/red-tests.cc:258: #if 0 Why? https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/red-tests.cc:284: Why? https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/codel-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/codel-queue-disc.cc:461: return (static_cast<uint32_t>(t.GetNanoSeconds () >> CODEL_SHIFT)); are parenthesis needed? return static_cast<uint32_t>(t.GetNanoSeconds () >> CODEL_SHIFT); https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/pie-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/pie-queue-disc.cc:220: m_dqCount = static_cast<uint32_t>(-1); After looking at the Linux code, I would do it this way: - define m_dqCount as uint64_t instead of uint32_t - define a static const uint64_t DQCOUNT_INVALID = std::numeric_limits<uint64_t>::max (); in class PieQueueDIsc - m_dqCount = DQCOUNT_INVALID; instead of m_dqCount = -1; https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/red-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/red-queue-disc.cc:672: double newAve = static_cast<double>(qAvg * pow(1.0-qW, m)); could you please instead try: double newAve = qAvg * std::pow (1.0-qW, m); https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc:359: queue->Enqueue (Create<AredQueueDiscTestItem> (Create<Packet> (size), dest, static_cast<uint16_t>(0))); Please remove the third parameter from the constructor of AredQueueDiscTestItem https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:70: * \param protocol Please remove this line: * \param protocol https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:458: uint32_t lowerBound = static_cast<uint32_t>(ns3Result - 0.02 * ns3Result); I would do it this way: uint32_t ns3Result = queue->ControlLaw (dropNextTestVals[i]); uint32_t linuxResult = _codel_control_law (queue, dropNextTestVals[i]); NS_TEST_EXPECT_MSG_EQ ((0.98 * ns3Result < linuxResult && linuxResult < 1.02 * ns3Result), true, "Linux result should stay within 2% of ns-3 result"); Please check if warnings are raised. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:535: NS_UNUSED(oldVal); I am not sure if consensus was reached on this. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/pie-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/pie-queue-disc-test-suite.cc:49: * \param protocol the protocol Please remove this line: * \param protocol the protocol https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/red-queue-disc-test-suite.cc:47: * \param protocol protocol Please remove this line: * \param protocol protocol
Sign in to reply to this message.
Updates for the latest review comments have been uploaded. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/adaptive-red-tests.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/adaptive-red-tests.cc:180: #if 0 On 2017/12/29 22:56:38, Stefano Avallone wrote: > Why? to help with the significant amount of testing being performed, I have updated all tests and examples to disable logging by default. I have standardized the approach used for logging that is not enabled via a command line option for all ns-3 tests and examples to use the same implementation (I copied what had been previously used elsewhere). https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/adaptive-red-tests.cc:206: On 2017/12/29 22:56:38, Stefano Avallone wrote: > Why? there are other Windows only changes coming and it made the source code read better to have this blank line. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/codel-vs-pfifo-basic-test.cc:91: float simDuration = 60; //in seconds On 2017/12/29 22:56:38, Stefano Avallone wrote: > please do not remove the white space, the comment is already aligned. restored, removal was a typo. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/pie-example.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/pie-example.cc:162: On 2017/12/29 22:56:38, Stefano Avallone wrote: > Why? changed for formatting. There is other Windows only changes that made the source code read better if this was inserted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... File src/traffic-control/examples/red-tests.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/red-tests.cc:258: #if 0 On 2017/12/29 22:56:38, Stefano Avallone wrote: > Why? see previous comment on changing all tests and examples to have logging disabled by default. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examp... src/traffic-control/examples/red-tests.cc:284: On 2017/12/29 22:56:38, Stefano Avallone wrote: > Why? see previous comment on future Windows only changes. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/codel-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/codel-queue-disc.cc:461: return (static_cast<uint32_t>(t.GetNanoSeconds () >> CODEL_SHIFT)); On 2017/12/29 22:56:38, Stefano Avallone wrote: > are parenthesis needed? > > return static_cast<uint32_t>(t.GetNanoSeconds () >> CODEL_SHIFT); changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/pie-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/pie-queue-disc.cc:220: m_dqCount = static_cast<uint32_t>(-1); On 2017/12/29 22:56:38, Stefano Avallone wrote: > After looking at the Linux code, I would do it this way: > > - define m_dqCount as uint64_t instead of uint32_t > - define a static const uint64_t DQCOUNT_INVALID = > std::numeric_limits<uint64_t>::max (); in class PieQueueDIsc > - m_dqCount = DQCOUNT_INVALID; instead of m_dqCount = -1; changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... File src/traffic-control/model/red-queue-disc.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model... src/traffic-control/model/red-queue-disc.cc:672: double newAve = static_cast<double>(qAvg * pow(1.0-qW, m)); On 2017/12/29 22:56:39, Stefano Avallone wrote: > could you please instead try: > > double newAve = qAvg * std::pow (1.0-qW, m); changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc:359: queue->Enqueue (Create<AredQueueDiscTestItem> (Create<Packet> (size), dest, static_cast<uint16_t>(0))); On 2017/12/29 22:56:39, Stefano Avallone wrote: > Please remove the third parameter from the constructor of AredQueueDiscTestItem changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:70: * \param protocol On 2017/12/29 22:56:39, Stefano Avallone wrote: > Please remove this line: > > * \param protocol changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:458: uint32_t lowerBound = static_cast<uint32_t>(ns3Result - 0.02 * ns3Result); On 2017/12/29 22:56:39, Stefano Avallone wrote: > I would do it this way: > > uint32_t ns3Result = queue->ControlLaw (dropNextTestVals[i]); > uint32_t linuxResult = _codel_control_law (queue, dropNextTestVals[i]); > NS_TEST_EXPECT_MSG_EQ ((0.98 * ns3Result < linuxResult && linuxResult < > 1.02 * ns3Result), true, > "Linux result should stay within 2% of ns-3 > result"); > > Please check if warnings are raised. changed as noted. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:535: NS_UNUSED(oldVal); On 2017/12/29 22:56:39, Stefano Avallone wrote: > I am not sure if consensus was reached on this. i am recommending these be kept since they have generated updates to resolve them in most cases by eliminating parameters where possible. the number of these left is a fairly small part of the changes. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/pie-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/pie-queue-disc-test-suite.cc:49: * \param protocol the protocol On 2017/12/29 22:56:39, Stefano Avallone wrote: > Please remove this line: > > * \param protocol the protocol removed. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/red-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/red-queue-disc-test-suite.cc:47: * \param protocol protocol On 2017/12/29 22:56:39, Stefano Avallone wrote: > Please remove this line: > > * \param protocol protocol changed as noted.
Sign in to reply to this message.
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:535: NS_UNUSED(oldVal); On 2017/12/31 05:15:03, ammo6818-vandals.uidaho.edu wrote: > On 2017/12/29 22:56:39, Stefano Avallone wrote: > > I am not sure if consensus was reached on this. > > i am recommending these be kept since they have generated updates to resolve > them in most cases by eliminating parameters where possible. the number of > these left is a fairly small part of the changes. Can you clarify? I did not understand the comment 'they have generated updates to resolve them...' Can this warning be suppressed in Visual Studio?
Sign in to reply to this message.
Response provided to Tom's review comments about unused parameters. https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... File src/traffic-control/test/codel-queue-disc-test-suite.cc (right): https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/... src/traffic-control/test/codel-queue-disc-test-suite.cc:535: NS_UNUSED(oldVal); On 2017/12/31 05:21:49, Tom Henderson wrote: > On 2017/12/31 05:15:03, http://ammo6818-vandals.uidaho.edu wrote: > > On 2017/12/29 22:56:39, Stefano Avallone wrote: > > > I am not sure if consensus was reached on this. > > > > i am recommending these be kept since they have generated updates to resolve > > them in most cases by eliminating parameters where possible. the number of > > these left is a fairly small part of the changes. > > Can you clarify? I did not understand the comment 'they have generated updates > to resolve them...' > > Can this warning be suppressed in Visual Studio? changes required to address these warnings have caused the reviewers to look at the code and determine that the code should be updated to remove the unused parameters. having these warnings visible (so they are not silent) has caused a code review by the maintainer. the only unused parameters that cannot be removed by code changes are unused parameters in callback functions and virtual functions, all other warnings can be eliminated by code changes. yes, this warning can be suppressed in Visual Studio. however, before going to that as a final decision I would recommend that the project process these changes and go through one iteration of changes to see if there is value in the warnings that were generated. if after that experience, the project feels there is more value in ignoring these warnings, the VS compile settings can be adjusted to ignore this type of warning at a later date. note, in the changes I have prepared, i have not tried to eliminate these warnings by source code changes, only add NS_UNUSED corrections to eliminate the warning. that is the same initial approach for the static_cast changes. based upon the maintainer review of the static cast changes, there have been a number of source code changes made by myself or suggested by the maintainer to eliminate the source of the need for the static cast. I would like to see the same approach used for the NS_UNUSED changes, the maintainer review each one and identify as a review comment those that should be architected out by making additional changes.
Sign in to reply to this message.
> > yes, this warning can be suppressed in Visual Studio. however, before going to > that as a final decision I would recommend that the project process these > changes and go through one iteration of changes to see if there is value in the > warnings that were generated. if after that experience, the project feels there > is more value in ignoring these warnings, the VS compile settings can be > adjusted to ignore this type of warning at a later date. > I support reviewing each one-by-one and fixing the unnecessary parameters. I'm mainly interested in avoiding future users of trace sources having to NS_UNUSED() the parameters that they choose to ignore.
Sign in to reply to this message.
All review comments have been addressed and are ready for merging into ns-3-dev.
Sign in to reply to this message.
Please let me know if there are any other review comments or merge patches into ns-3-dev
Sign in to reply to this message.
Pushed to ns-3-dev with changeset 13307:111e0d0cb0ca. Thanks, Stefano
Sign in to reply to this message.
There is one more change required to eliminate the final warning. Much thanks for your effort and support on this.
Sign in to reply to this message.
|