Code review - Issue 333080043: Eliminate Visual Studio complier warningshttps://codereview.appspot.com/2018-03-01T10:46:14+00:00rietveld
Message from unknown
2017-11-10T21:41:20+00:00ammo6818-vandals.uidaho.eduurn:md5:69f30c9655238319c8f3f9ef93b8f71a
Message from unknown
2017-12-09T01:52:01+00:00ammo6818-vandals.uidaho.eduurn:md5:b51a2c4521ca92488ee5f520773374e4
Message from stavallo@gmail.com
2017-12-28T23:55:28+00:00Stefano Avalloneurn:md5:a9d07bc8983507742b31a4af504b0bf5
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc
File src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/codel-vs-pfifo-asymmetric.cc#newcode235
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/examples/codel-vs-pfifo-basic-test.cc
File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/codel-vs-pfifo-basic-test.cc#newcode90
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/examples/pfifo-vs-red.cc
File src/traffic-control/examples/pfifo-vs-red.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/pfifo-vs-red.cc#newcode174
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/helper/traffic-control-helper.cc
File src/traffic-control/helper/traffic-control-helper.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helper/traffic-control-helper.cc#newcode88
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/fq-codel-queue-disc.cc
File src/traffic-control/model/fq-codel-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model/fq-codel-queue-disc.cc#newcode282
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/traffic-control-layer.cc
File src/traffic-control/model/traffic-control-layer.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model/traffic-control-layer.cc#newcode109
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/traffic-control-layer.cc#newcode119
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/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode201
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/pie-queue-disc-test-suite.cc
File src/traffic-control/test/pie-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/pie-queue-disc-test-suite.cc#newcode183
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/red-queue-disc-test-suite.cc
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/red-queue-disc-test-suite.cc#newcode174
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.
Message from womenrgr8id@gmail.com
2017-12-29T04:43:19+00:00ammo6818-vandals.uidaho.eduurn:md5:5bff9e1d5c1ee0bbfc4c8ef43e27b25e
Updated patch set has been uploaded to address your review comments.
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/pfifo-vs-red.cc
File src/traffic-control/examples/pfifo-vs-red.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/examples/pfifo-vs-red.cc#newcode174
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/helper/traffic-control-helper.cc
File src/traffic-control/helper/traffic-control-helper.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/helper/traffic-control-helper.cc#newcode88
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/fq-codel-queue-disc.cc
File src/traffic-control/model/fq-codel-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/model/fq-codel-queue-disc.cc#newcode282
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/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode201
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/pie-queue-disc-test-suite.cc
File src/traffic-control/test/pie-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/pie-queue-disc-test-suite.cc#newcode183
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/red-queue-disc-test-suite.cc
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/20001/src/traffic-control/test/red-queue-disc-test-suite.cc#newcode174
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.
Message from unknown
2017-12-29T04:46:33+00:00ammo6818-vandals.uidaho.eduurn:md5:6221d713ad76f67bc7047efa4487fc56
Message from stavallo@gmail.com
2017-12-29T22:56:39+00:00Stefano Avalloneurn:md5:470d995830929b4889badc511c03ff50
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/adaptive-red-tests.cc
File src/traffic-control/examples/adaptive-red-tests.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/adaptive-red-tests.cc#newcode180
src/traffic-control/examples/adaptive-red-tests.cc:180: #if 0
Why?
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/adaptive-red-tests.cc#newcode206
src/traffic-control/examples/adaptive-red-tests.cc:206:
Why?
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/codel-vs-pfifo-basic-test.cc
File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/codel-vs-pfifo-basic-test.cc#newcode91
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/examples/pie-example.cc
File src/traffic-control/examples/pie-example.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/pie-example.cc#newcode162
src/traffic-control/examples/pie-example.cc:162:
Why?
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/red-tests.cc
File src/traffic-control/examples/red-tests.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/red-tests.cc#newcode258
src/traffic-control/examples/red-tests.cc:258: #if 0
Why?
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/red-tests.cc#newcode284
src/traffic-control/examples/red-tests.cc:284:
Why?
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/codel-queue-disc.cc
File src/traffic-control/model/codel-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/codel-queue-disc.cc#newcode461
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/pie-queue-disc.cc
File src/traffic-control/model/pie-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/pie-queue-disc.cc#newcode220
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/red-queue-disc.cc
File src/traffic-control/model/red-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/red-queue-disc.cc#newcode672
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/adaptive-red-queue-disc-test-suite.cc
File src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc#newcode359
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/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode70
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/codel-queue-disc-test-suite.cc#newcode458
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/codel-queue-disc-test-suite.cc#newcode535
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/pie-queue-disc-test-suite.cc
File src/traffic-control/test/pie-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/pie-queue-disc-test-suite.cc#newcode49
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/red-queue-disc-test-suite.cc
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/red-queue-disc-test-suite.cc#newcode47
src/traffic-control/test/red-queue-disc-test-suite.cc:47: * \param protocol protocol
Please remove this line:
* \param protocol protocol
Message from unknown
2017-12-31T05:14:29+00:00ammo6818-vandals.uidaho.eduurn:md5:12e27a540227f4f98949df42d6377f8b
Message from womenrgr8id@gmail.com
2017-12-31T05:15:04+00:00ammo6818-vandals.uidaho.eduurn:md5:d195609d9b941bd7d3c76319342074dc
Updates for the latest review comments have been uploaded.
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/adaptive-red-tests.cc
File src/traffic-control/examples/adaptive-red-tests.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/adaptive-red-tests.cc#newcode180
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/examples/adaptive-red-tests.cc#newcode206
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/examples/codel-vs-pfifo-basic-test.cc
File src/traffic-control/examples/codel-vs-pfifo-basic-test.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/codel-vs-pfifo-basic-test.cc#newcode91
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/examples/pie-example.cc
File src/traffic-control/examples/pie-example.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/pie-example.cc#newcode162
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/examples/red-tests.cc
File src/traffic-control/examples/red-tests.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/examples/red-tests.cc#newcode258
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/examples/red-tests.cc#newcode284
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/codel-queue-disc.cc
File src/traffic-control/model/codel-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/codel-queue-disc.cc#newcode461
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/pie-queue-disc.cc
File src/traffic-control/model/pie-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/pie-queue-disc.cc#newcode220
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/red-queue-disc.cc
File src/traffic-control/model/red-queue-disc.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/model/red-queue-disc.cc#newcode672
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/adaptive-red-queue-disc-test-suite.cc
File src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/adaptive-red-queue-disc-test-suite.cc#newcode359
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/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode70
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/codel-queue-disc-test-suite.cc#newcode458
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/codel-queue-disc-test-suite.cc#newcode535
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/pie-queue-disc-test-suite.cc
File src/traffic-control/test/pie-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/pie-queue-disc-test-suite.cc#newcode49
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/red-queue-disc-test-suite.cc
File src/traffic-control/test/red-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/red-queue-disc-test-suite.cc#newcode47
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.
Message from tomh.org@gmail.com
2017-12-31T05:21:49+00:00Tom Hendersonurn:md5:a226749548378dd051da76923b5aba7c
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode535
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?
Message from womenrgr8id@gmail.com
2017-12-31T15:21:45+00:00ammo6818-vandals.uidaho.eduurn:md5:c17b23a6bf7ad9c35b68ffd799d8f91a
Response provided to Tom's review comments about unused parameters.
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc
File src/traffic-control/test/codel-queue-disc-test-suite.cc (right):
https://codereview.appspot.com/333080043/diff/60001/src/traffic-control/test/codel-queue-disc-test-suite.cc#newcode535
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.
Message from tomh.org@gmail.com
2017-12-31T18:57:21+00:00Tom Hendersonurn:md5:cd9172f91f94b749803db92e2c0bbb99
>
> 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.
Message from unknown
2018-01-15T02:26:58+00:00ammo6818-vandals.uidaho.eduurn:md5:94a7c6f5c92961559331684b6c8d0d77
Message from womenrgr8id@gmail.com
2018-02-11T19:43:38+00:00ammo6818-vandals.uidaho.eduurn:md5:502e337c7fea0debeb261e800c586b1e
All review comments have been addressed and are ready for merging into ns-3-dev.
Message from womenrgr8id@gmail.com
2018-02-18T05:42:34+00:00ammo6818-vandals.uidaho.eduurn:md5:59b541fc2422746f396e47d11be3e1a8
Please let me know if there are any other review comments or merge patches into ns-3-dev
Message from stavallo@gmail.com
2018-02-23T14:48:06+00:00Stefano Avalloneurn:md5:ec43623cb91621e1f2c9a764d7844392
Pushed to ns-3-dev with changeset 13307:111e0d0cb0ca.
Thanks,
Stefano
Message from unknown
2018-02-28T20:32:20+00:00ammo6818-vandals.uidaho.eduurn:md5:fcb8bcda590a744c15e4a4fcc31a1917
Message from womenrgr8id@gmail.com
2018-02-28T20:33:43+00:00ammo6818-vandals.uidaho.eduurn:md5:d28466041dd6e39e253f5c07634db333
There is one more change required to eliminate the final warning.
Much thanks for your effort and support on this.
Message from stavallo@gmail.com
2018-03-01T10:46:14+00:00Stefano Avalloneurn:md5:7955c8b790561a8b2d44132592ba2c51
Yeah, I just pushed a fix that avoids casting and breaking build with clang.
Thanks.