Code review - Issue 334560043: Eliminate Visual Studio compiler warningshttps://codereview.appspot.com/2018-04-03T20:25:33+00:00rietveld
Message from unknown
2018-03-09T11:33:36+00:00ammo6818-vandals.uidaho.eduurn:md5:39a06669233d2e745448ec6426d8947b
Message from sebastien.deronne@gmail.com
2018-03-09T12:16:18+00:00S. Deronneurn:md5:800e94c28699190896846dc3af6e3625
I see plenty of casts added, 2 questions:
1/ Why? How can we avoid so much casts?
2/ Why did they pop up right now? I have not seen that much casts in your previous patch sets.
Message from tomh.org@gmail.com
2018-03-09T16:56:40+00:00Tom Hendersonurn:md5:93bd0965c3a3d93427e81a81b04a7eb3
I did not review each one but I think that most if not all casts can be avoided by changing the variable type.
I also do not want to ifdef out the log statements; Robert, why is this needed for Visual Studio? (apologies for asking again, I think you explained some time ago but I forgot)
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc
File examples/wireless/80211e-txop.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc#newcode226
examples/wireless/80211e-txop.cc:226: serverAppA.Stop (Seconds (static_cast<double> (simulationTime + 1)));
This cast can be avoided by changing the type of simulationTime
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc#newcode312
examples/wireless/80211e-txop.cc:312: uint32_t totalPacketsThrough = static_cast<uint32_t> (DynamicCast<UdpServer> (serverAppA.Get (0))->GetReceived ());
This type can be avoided by changing the type of totalPacketsThrough.
https://codereview.appspot.com/334560043/diff/1/examples/wireless/ofdm-ht-validation.cc
File examples/wireless/ofdm-ht-validation.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/ofdm-ht-validation.cc#newcode47
examples/wireless/ofdm-ht-validation.cc:47: CommandLine cmd;
This is a whitespace change.
https://codereview.appspot.com/334560043/diff/1/examples/wireless/wifi-blockack.cc
File examples/wireless/wifi-blockack.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/wifi-blockack.cc#newcode62
examples/wireless/wifi-blockack.cc:62: #endif
As I've commented before, this should not be disabled; if necessary to have it be disabled, provide a 'quiet' command line argument (default to false).
Message from unknown
2018-03-11T16:45:39+00:00ammo6818-vandals.uidaho.eduurn:md5:70046bcb88ffed622ef7a91068c3f22a
Message from unknown
2018-03-11T18:52:38+00:00ammo6818-vandals.uidaho.eduurn:md5:d4f5420aa78fa86ae559db597e6f7701
Message from womenrgr8id@gmail.com
2018-03-11T19:00:44+00:00ammo6818-vandals.uidaho.eduurn:md5:4da7be40ba202760833dc5308d5a32fd
On 2018/03/09 12:16:18, S. Deronne wrote:
> I see plenty of casts added, 2 questions:
>
> 1/ Why? How can we avoid so much casts?
> 2/ Why did they pop up right now? I have not seen that much casts in your
> previous patch sets.
The conservative change is to add a cast rather than try to reword the code. Adding a cast reduces the possibility of side effects from a code change. All changes made initially for all modules was to add the cast. Based upon your comments, the code has been modified to minimize the number of casts.
Message from womenrgr8id@gmail.com
2018-03-11T19:01:18+00:00ammo6818-vandals.uidaho.eduurn:md5:b1584f731cab1a8a7620b8baf7312950
Review comments incorporated.
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc
File examples/wireless/80211e-txop.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc#newcode226
examples/wireless/80211e-txop.cc:226: serverAppA.Stop (Seconds (static_cast<double> (simulationTime + 1)));
On 2018/03/09 16:56:40, Tom Henderson wrote:
> This cast can be avoided by changing the type of simulationTime
Changed to eliminate cast
https://codereview.appspot.com/334560043/diff/1/examples/wireless/80211e-txop.cc#newcode312
examples/wireless/80211e-txop.cc:312: uint32_t totalPacketsThrough = static_cast<uint32_t> (DynamicCast<UdpServer> (serverAppA.Get (0))->GetReceived ());
On 2018/03/09 16:56:40, Tom Henderson wrote:
> This type can be avoided by changing the type of totalPacketsThrough.
Changed to eliminate cast
https://codereview.appspot.com/334560043/diff/1/examples/wireless/ofdm-ht-validation.cc
File examples/wireless/ofdm-ht-validation.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/ofdm-ht-validation.cc#newcode47
examples/wireless/ofdm-ht-validation.cc:47: CommandLine cmd;
On 2018/03/09 16:56:40, Tom Henderson wrote:
> This is a whitespace change.
Change reverted.
https://codereview.appspot.com/334560043/diff/1/examples/wireless/wifi-blockack.cc
File examples/wireless/wifi-blockack.cc (right):
https://codereview.appspot.com/334560043/diff/1/examples/wireless/wifi-blockack.cc#newcode62
examples/wireless/wifi-blockack.cc:62: #endif
On 2018/03/09 16:56:40, Tom Henderson wrote:
> As I've commented before, this should not be disabled; if necessary to have it
> be disabled, provide a 'quiet' command line argument (default to false).
Change reverted.
Message from sebastien.deronne@gmail.com
2018-03-27T10:36:17+00:00S. Deronneurn:md5:11bca78b61b74813a262a6063fc75712
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/80211e-txop.cc
File examples/wireless/80211e-txop.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/80211e-txop.cc#newcode62
examples/wireless/80211e-txop.cc:62: double simulationTime = 10.0; //seconds
is .0 needed? If yes, then the following argument should be 5.0
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/80211n-mimo.cc
File examples/wireless/80211n-mimo.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/80211n-mimo.cc#newcode263
examples/wireless/80211n-mimo.cc:263: d += static_cast<int> (step);
better to define d as a double?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/power-adaptation-distance.cc
File examples/wireless/power-adaptation-distance.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/power-adaptation-distance.cc#newcode350
examples/wireless/power-adaptation-distance.cc:350: wifi.SetRemoteStationManager (manager, "DefaultTxPowerLevel", UintegerValue (static_cast<uint64_t> (maxPower)), "RtsCtsThreshold", UintegerValue (rtsThreshold));
why you don't need a cast in other UintegerValue?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-hidden-terminal.cc
File examples/wireless/wifi-hidden-terminal.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-hidden-terminal.cc#newcode55
examples/wireless/wifi-hidden-terminal.cc:55: for (uint32_t i = 0; i < 3; ++i)
it can't be uint8_t?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-per-example.cc
File examples/wireless/wifi-spectrum-per-example.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-per-example.cc#newcode528
examples/wireless/wifi-spectrum-per-example.cc:528: uint32_t totalBytesRx = static_cast<uint32_t> (DynamicCast<PacketSink> (serverApp.Get (0))->GetTotalRx ());
use uint64_t as for other examples?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-per-interference.cc
File examples/wireless/wifi-spectrum-per-interference.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-per-interference.cc#newcode580
examples/wireless/wifi-spectrum-per-interference.cc:580: totalPacketsThrough = static_cast<uint32_t> (DynamicCast<UdpServer> (serverApp.Get (0))->GetReceived ());
use uint64_t as for other examples?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-per-interference.cc#newcode586
examples/wireless/wifi-spectrum-per-interference.cc:586: uint32_t totalBytesRx = static_cast<uint32_t> (DynamicCast<PacketSink> (serverApp.Get (0))->GetTotalRx ());
use uint64_t as for other examples?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-saturation-example.cc
File examples/wireless/wifi-spectrum-saturation-example.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-spectrum-saturation-example.cc#newcode691
examples/wireless/wifi-spectrum-saturation-example.cc:691: totalPacketsThrough = static_cast<uint32_t> (DynamicCast<UdpServer> (serverApp.Get (0))->GetReceived ());
use uint64_t as for other examples?
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-timing-attributes.cc
File examples/wireless/wifi-timing-attributes.cc (right):
https://codereview.appspot.com/334560043/diff/40001/examples/wireless/wifi-timing-attributes.cc#newcode165
examples/wireless/wifi-timing-attributes.cc:165: uint32_t totalPacketsThrough = static_cast<uint32_t> (DynamicCast<UdpServer> (serverApp.Get (0))->GetReceived ());
use uint64_t as for other examples?
Message from sebastien.deronne@gmail.com
2018-04-03T20:25:33+00:00S. Deronneurn:md5:029f3d6b143ce1ab8f68639c17c0d89f
Robert, could you update with the latest ns-3-dev and address my comments?
Thanks.