Eliminate Visual Studio compiler warnings

Eliminate Visual Studio compiler warnings

[S. Deronne, 2017-12-17 15:39:16]

Robert, thanks for your huge work. I added some comments inline. I am also not
fully convinced all casts are needed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/examples/test-in...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:121: txVector.SetTxPowerLevel
(static_cast<int8_t>(m_input.txPowerLevelA));
why int8_t? I think it is uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:396: rates.AddSupportedRate
(static_cast<uint32_t>(modeDataRate));
32 or 64 bits?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:412: rates.SetBasicRate
(static_cast<uint32_t>(modeDataRate));
same comment

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-cache.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-cache.cc:39: m_winSize = winSize <= 64 ?
static_cast<uint8_t>(winSize) : 64;
why uint8_t?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-manager.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-manager.cc:411: 
are those changes needed?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/he-capabil...
File src/wifi/model/he-capabilities.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/he-capabil...
src/wifi/model/he-capabilities.cc:462: {
I guess mcs input parameter can be removed

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
File src/wifi/model/ht-operation.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
src/wifi/model/ht-operation.cc:340: return static_cast<uint8_t>(val);
val should have the correct number of bits, I don't think a cast is the solution
here

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
src/wifi/model/ht-operation.cc:481: uint8_t informationsubset3 =
static_cast<uint8_t>(i.ReadU16 ());
there is something strange here, we read 16 bits but we store in 8 bits...

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:127: mode = GetPhy ()->GetMcs
(static_cast<uint8_t>(i));
can't we simply define i and j as uint8_t???

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:307: selectedNss =
static_cast<uint8_t>(station->m_nss);
why a cast?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:2547: 
is this needed?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1000: NS_UNUSED(to);
I guess to should be removed from input parameters.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1134: NS_UNUSED(preamble);
I guess preamble should be removed from input parameters.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1749: NS_UNUSED(source);
I guess source should be removed from input parameters.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1890: WifiMode mode = phy->GetMcs
(static_cast<uint8_t>(i));
simply define i as uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1906: WifiMode mode = phy->GetMcs
(static_cast<uint8_t>(i));
same here

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
File src/wifi/model/mpdu-standard-aggregator.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
src/wifi/model/mpdu-standard-aggregator.cc:85: currentHdr.SetLength
(static_cast<uint8_t>(packet->GetSize ()));
16 bits?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/regular-wi...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/regular-wi...
src/wifi/model/regular-wifi-mac.cc:992: NS_UNUSED(hdr);
hdr can certainly be removed from input parameters

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/rraa-wifi-...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/rraa-wifi-...
src/wifi/model/rraa-wifi-manager.cc:280: NS_UNUSED(station);
to be checked what is the purpose of this dummy function

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
src/wifi/model/spectrum-wifi-phy.cc:387: return
static_cast<uint32_t>(guardBandwidth);
32 or 8 bits for guardBandwidth?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
File src/wifi/model/sta-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:580: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
i should be uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:596: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
same here

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:829: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
idem

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:842: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
idem

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:855: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
idem

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
File src/wifi/model/vht-capabilities.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:115: uint16_t vhtinfo =
static_cast<uint16_t>(i.ReadLsbtohU32 ());
very strange here, we read 32 bits but we store in 16 bits

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:327: return
static_cast<uint8_t>(m_maxMpduLength);
not ok, m_maxMpduLength should be 8 bits

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:333: return
static_cast<uint8_t>(m_supportedChannelWidthSet);
not ok, m_supportedChannelWidthSet should be 8 bits

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:729: return static_cast<uint8_t>(m_qosStuff);
not ok, m_qosStuff should be defined as uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-phy.c...
src/wifi/model/wifi-phy.cc:3535: if (mcs == GetMcs (static_cast<uint8_t>(i)))
why is this cast needed?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
File src/wifi/model/wifi-remote-station-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
src/wifi/model/wifi-remote-station-manager.cc:682:
state->m_operationalMcsSet.push_back (m_wifiPhy->GetMcs
(static_cast<uint8_t>(i)));
i should be uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
src/wifi/model/wifi-remote-station-manager.cc:1423: WifiMode thismode =
m_wifiPhy->GetMcs (static_cast<uint8_t>(idx));
idx should be uint8_t

[S. Deronne, 2017-12-17 16:02:45]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:979: {
i should be coded on 8 bits to avoid casts

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:1082: {
j should be coded on 8 bits to avoid casts

[S. Deronne, 2017-12-17 16:18:27]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.cc:211: NS_UNUSED(preamble);
we can simply add preamble in the log print as for other parameters

[S. Deronne, 2017-12-17 16:24:21]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/dsss-param...
File src/wifi/model/dsss-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/dsss-param...
src/wifi/model/dsss-parameter-set.cc:22: #include "ns3/unused.h"
needed?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/edca-param...
File src/wifi/model/edca-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/edca-param...
src/wifi/model/edca-parameter-set.cc:22: #include "ns3/unused.h"
needed?

[S. Deronne, 2017-12-17 16:49:29]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:69: {
why no unused needed for the duration?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/regular-wi...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/regular-wi...
src/wifi/model/regular-wifi-mac.cc:992: NS_UNUSED(hdr);
On 2017/12/17 15:39:15, S. Deronne wrote:
> hdr can certainly be removed from input parameters

hdr cannot be removed, so let's keep this NS_UNUSED

[S. Deronne, 2017-12-17 17:07:37]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.c...
src/wifi/test/wifi-test.cc:586: NS_UNUSED(adr);
adr can be removed

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.c...
src/wifi/test/wifi-test.cc:1067: NS_UNUSED(adr);
adr can be removed

[S. Deronne, 2017-12-17 17:14:58]

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
File src/wifi/test/block-ack-test-suite.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
src/wifi/test/block-ack-test-suite.cc:274: for (uint16_t i = 179; i < 220; i++)
can be uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
src/wifi/test/block-ack-test-suite.cc:298: for (uint16_t i = 22; i < 25; i++)
can be uint8_t

[ammo6818-vandals.uidaho.edu, 2017-12-20 04:16:35]

An updated patch has been uploaded to address your review comments.  I have also
replied to each comment individually.

Much thanks for your review since it found at least 5 errors in the code.

Please merge changes to ns-3-dev at your earliest convenience.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.cc:211: NS_UNUSED(preamble);
On 2017/12/17 16:18:27, S. Deronne wrote:
> we can simply add preamble in the log print as for other parameters

modified as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:979: {
On 2017/12/17 16:02:45, S. Deronne wrote:
> i should be coded on 8 bits to avoid casts

GetBasicMode() expects a uint32_t for the parameter.  changing i will not reduce
the type casts.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:1082: {
On 2017/12/17 16:02:45, S. Deronne wrote:
> j should be coded on 8 bits to avoid casts

Changed as noted

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:396: rates.AddSupportedRate
(static_cast<uint32_t>(modeDataRate));
On 2017/12/17 15:39:14, S. Deronne wrote:
> 32 or 64 bits?

The parameter to AddSupportedRate takes a uint32_t parameter

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:412: rates.SetBasicRate
(static_cast<uint32_t>(modeDataRate));
On 2017/12/17 15:39:14, S. Deronne wrote:
> same comment

The parameter to AddSupportedRate takes a uint32_t parameter

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-cache.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-cache.cc:39: m_winSize = winSize <= 64 ?
static_cast<uint8_t>(winSize) : 64;
On 2017/12/17 15:39:14, S. Deronne wrote:
> why uint8_t?

m_winSize is uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-manager.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-manager.cc:411: 
On 2017/12/17 15:39:14, S. Deronne wrote:
> are those changes needed?

tid and recipient are parameters to this function.  reusing the same variable
names hides the previous ones which generates a warning in Visual Studio.  Per
comments from Natalie on the original NS-3 submission, these need to be
corrected to eliminate the variable hiding.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/dsss-param...
File src/wifi/model/dsss-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/dsss-param...
src/wifi/model/dsss-parameter-set.cc:22: #include "ns3/unused.h"
On 2017/12/17 16:24:20, S. Deronne wrote:
> needed?

removed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/edca-param...
File src/wifi/model/edca-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/edca-param...
src/wifi/model/edca-parameter-set.cc:22: #include "ns3/unused.h"
On 2017/12/17 16:24:20, S. Deronne wrote:
> needed?

removed

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
File src/wifi/model/ht-operation.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
src/wifi/model/ht-operation.cc:340: return static_cast<uint8_t>(val);
On 2017/12/17 15:39:14, S. Deronne wrote:
> val should have the correct number of bits, I don't think a cast is the
solution
> here

val is defined as uint16_t while the function returns uint8_t.  Patch changed to
declare val as uint8_t and cast removed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ht-operati...
src/wifi/model/ht-operation.cc:481: uint8_t informationsubset3 =
static_cast<uint8_t>(i.ReadU16 ());
On 2017/12/17 15:39:14, S. Deronne wrote:
> there is something strange here, we read 16 bits but we store in 8 bits...

informationsubset2 and informationsubset3 change to uint16_t and cast removed

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:127: mode = GetPhy ()->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:14, S. Deronne wrote:
> can't we simply define i and j as uint8_t???

changed as noted for i
j must be a uint16_t in the loop control variable because the loop completion
value is 160*2 so we need a unit16_t for the loop control then cast j as the
parameter.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:307: selectedNss =
static_cast<uint8_t>(station->m_nss);
On 2017/12/17 15:39:15, S. Deronne wrote:
> why a cast?

selectedNss is a uint8_t while m_nss is adouble

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:69: {
On 2017/12/17 16:49:28, S. Deronne wrote:
> why no unused needed for the duration?

unknown, for whatever reason, VS does not generate a warning.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:2547: 
On 2017/12/17 15:39:15, S. Deronne wrote:
> is this needed?

this is an example of variable hiding.  for all those cases, I took the
conservative approach of creating a new variable rather than reusing the
existing variable in case the existing variable value was used later on in the
code.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1000: NS_UNUSED(to);
On 2017/12/17 15:39:15, S. Deronne wrote:
> I guess to should be removed from input parameters.

removed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1134: NS_UNUSED(preamble);
On 2017/12/17 15:39:15, S. Deronne wrote:
> I guess preamble should be removed from input parameters.

removed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1749: NS_UNUSED(source);
On 2017/12/17 15:39:15, S. Deronne wrote:
> I guess source should be removed from input parameters.

removed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1890: WifiMode mode = phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> simply define i as uint8_t

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1906: WifiMode mode = phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> same here

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
File src/wifi/model/mpdu-standard-aggregator.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
src/wifi/model/mpdu-standard-aggregator.cc:85: currentHdr.SetLength
(static_cast<uint8_t>(packet->GetSize ()));
On 2017/12/17 15:39:15, S. Deronne wrote:
> 16 bits?

comments ignored, this file has been removed

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/rraa-wifi-...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/rraa-wifi-...
src/wifi/model/rraa-wifi-manager.cc:280: NS_UNUSED(station);
On 2017/12/17 15:39:15, S. Deronne wrote:
> to be checked what is the purpose of this dummy function

function removed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
src/wifi/model/spectrum-wifi-phy.cc:387: return
static_cast<uint32_t>(guardBandwidth);
On 2017/12/17 15:39:15, S. Deronne wrote:
> 32 or 8 bits for guardBandwidth?

guardBandwidth changed to uint32_t from double

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
File src/wifi/model/sta-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:580: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> i should be uint8_t

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:596: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> same here

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:829: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> idem

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:842: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> idem

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/sta-wifi-m...
src/wifi/model/sta-wifi-mac.cc:855: WifiMode mcs = m_phy->GetMcs
(static_cast<uint8_t>(i));
On 2017/12/17 15:39:15, S. Deronne wrote:
> idem

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
File src/wifi/model/vht-capabilities.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:115: uint16_t vhtinfo =
static_cast<uint16_t>(i.ReadLsbtohU32 ());
On 2017/12/17 15:39:15, S. Deronne wrote:
> very strange here, we read 32 bits but we store in 16 bits

vhtinfo changed to uint32_t and cast removed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:327: return
static_cast<uint8_t>(m_maxMpduLength);
On 2017/12/17 15:39:15, S. Deronne wrote:
> not ok, m_maxMpduLength should be 8 bits

m_maxMpduLength changed from uint16_t to uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/vht-capabi...
src/wifi/model/vht-capabilities.cc:333: return
static_cast<uint8_t>(m_supportedChannelWidthSet);
On 2017/12/17 15:39:15, S. Deronne wrote:
> not ok, m_supportedChannelWidthSet should be 8 bits

m_supportedChannelWidthSet changed from uint16_t to uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:729: return static_cast<uint8_t>(m_qosStuff);
On 2017/12/17 15:39:15, S. Deronne wrote:
> not ok, m_qosStuff should be defined as uint8_t

m_qosStuff changed from uint16_t to uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-phy.c...
src/wifi/model/wifi-phy.cc:3535: if (mcs == GetMcs (static_cast<uint8_t>(i)))
On 2017/12/17 15:39:16, S. Deronne wrote:
> why is this cast needed?

removed.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
File src/wifi/model/wifi-remote-station-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
src/wifi/model/wifi-remote-station-manager.cc:682:
state->m_operationalMcsSet.push_back (m_wifiPhy->GetMcs
(static_cast<uint8_t>(i)));
On 2017/12/17 15:39:16, S. Deronne wrote:
> i should be uint8_t

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/wifi-remot...
src/wifi/model/wifi-remote-station-manager.cc:1423: WifiMode thismode =
m_wifiPhy->GetMcs (static_cast<uint8_t>(idx));
On 2017/12/17 15:39:16, S. Deronne wrote:
> idx should be uint8_t

changed as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
File src/wifi/test/block-ack-test-suite.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
src/wifi/test/block-ack-test-suite.cc:274: for (uint16_t i = 179; i < 220; i++)
On 2017/12/17 17:14:58, S. Deronne wrote:
> can be uint8_t

the function expects a uint16_t parameter

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/block-ack-t...
src/wifi/test/block-ack-test-suite.cc:298: for (uint16_t i = 22; i < 25; i++)
On 2017/12/17 17:14:58, S. Deronne wrote:
> can be uint8_t

the function expects a uint16_t parameter

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.c...
src/wifi/test/wifi-test.cc:586: NS_UNUSED(adr);
On 2017/12/17 17:07:36, S. Deronne wrote:
> adr can be removed

this is a callback function and the parameter is required.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.c...
src/wifi/test/wifi-test.cc:1067: NS_UNUSED(adr);
On 2017/12/17 17:07:36, S. Deronne wrote:
> adr can be removed

this is a callback function and the parameter is required.

[S. Deronne, 2017-12-22 22:28:28]

Could you please provide an updated patch based on the latest ns-3-dev?
As you might have noticed, I started to push some cleanup in ns-3-dev to remove
the need for all those casts. I will already deliver some parts of your changes
that are fine for me.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:979: {
see my changes to the mainstream, GetNBasicModes now return uint8_t, so no need
for cast anymore.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:396: rates.AddSupportedRate
(static_cast<uint32_t>(modeDataRate));
maybe AddSupportedRate should take uint64_t as input, and then no need for cast.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-cache.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-cache.cc:39: m_winSize = winSize <= 64 ?
static_cast<uint8_t>(winSize) : 64;
then why not simply changing input parameter to uint8_t if this is sufficient?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-manager.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-manager.cc:411: 
I do not like it that much... Maybe we could use something like this:

{
    uint8_t tid = hdr.GetQosTid ();
    ...
}

{
    uint8_t tid = hdr.GetQosTid ();
    ...
}

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:127: mode = GetPhy ()->GetMcs
(static_cast<uint8_t>(i));
Correct. I think by changing the loop we could get something working, I will
have a look.
I am thinking to an exiting flag, set to true when channel width is 160 so that
it exits at the next iteration.
I want to avoid as much as possible to have cast in the code.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:307: selectedNss =
static_cast<uint8_t>(station->m_nss);
m_nss should be moved to an uint8_t

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:69: {
Then I suggest to add it anyway, for keeping our code consistent.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:2547: 
I want to find another alternative

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
File src/wifi/model/mpdu-standard-aggregator.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
src/wifi/model/mpdu-standard-aggregator.cc:85: currentHdr.SetLength
(static_cast<uint8_t>(packet->GetSize ()));
It has just be moved, so still valid.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/spectrum-w...
src/wifi/model/spectrum-wifi-phy.cc:387: return
static_cast<uint32_t>(guardBandwidth);
On 2017/12/20 04:16:34, ammo6818-vandals.uidaho.edu wrote:
> On 2017/12/17 15:39:15, S. Deronne wrote:
> > 32 or 8 bits for guardBandwidth?
> 
> guardBandwidth changed to uint32_t from double

?

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/test/wifi-test.c...
src/wifi/test/wifi-test.cc:586: NS_UNUSED(adr);
On 2017/12/20 04:16:35, ammo6818-vandals.uidaho.edu wrote:
> On 2017/12/17 17:07:36, S. Deronne wrote:
> > adr can be removed
> 
> this is a callback function and the parameter is required.

Indeed, you're right.

[S. Deronne, 2017-12-23 10:19:59]

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:121: txVector.SetTxPowerLevel
(static_cast<uint8_t>(m_input.txPowerLevelA));
this is not correct initially, they set the power in dbm to a MIB value. IMO it
can simply be removed, I will give a try.

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:132: txVector.SetTxPowerLevel
(static_cast<uint8_t>(m_input.txPowerLevelB));
same comment

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:277: input.interval = MicroSeconds
(static_cast<uint64_t>(delay));
why?

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/wifi-ma...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/wifi-ma...
src/wifi/examples/wifi-manager-example.cc:319: steps =
static_cast<uint32_t>(std::abs ((int) (clientSelectedStandard.m_snrHigh -
clientSelectedStandard.m_snrLow ) / stepSize) + 1);
I see two cast here, one c++ and one old C style cast. Can't we have something
better?

https://codereview.appspot.com/333070043/diff/80001/src/wifi/helper/wifi-help...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/helper/wifi-help...
src/wifi/helper/wifi-helper.cc:210: rate =
static_cast<uint32_t>(txVector.GetMode ().GetDataRate (txVector.GetChannelWidth
(), txVector.GetGuardInterval (), 1) * txVector.GetNss () / 500000);
I do not have a direct solution for that, I noticed some issues with high
datarate, I have to check how to fix this.

https://codereview.appspot.com/333070043/diff/80001/src/wifi/helper/wifi-help...
src/wifi/helper/wifi-helper.cc:507: header.SetAmpduStatus
(static_cast<uint32_t>(aMpdu.mpduRefNumber), ampduStatusFlags, hdr.GetCrc ());
mpduRefNumber should be limited to uint32_t

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
File src/wifi/test/tx-duration-test.cc (left):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
src/wifi/test/tx-duration-test.cc:109: Time calculatedDuration =
phy->GetPayloadDuration (size, txVector, testedFrequency);
testedFrequency should be uint16_t

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
File src/wifi/test/tx-duration-test.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
src/wifi/test/tx-duration-test.cc:95: txVector.SetChannelWidth
(static_cast<uint8_t>(channelWidth));
channelWidth should be uint8_t

[ammo6818-vandals.uidaho.edu, 2017-12-23 15:14:33]

Additional changes for your latest review comments.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:979: {
On 2017/12/22 22:28:21, S. Deronne wrote:
> see my changes to the mainstream, GetNBasicModes now return uint8_t, so no
need
> for cast anymore.

Updated as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:396: rates.AddSupportedRate
(static_cast<uint32_t>(modeDataRate));
On 2017/12/22 22:28:21, S. Deronne wrote:
> maybe AddSupportedRate should take uint64_t as input, and then no need for
cast.

implemented this change.  also modified the other affected functions.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-cache.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-cache.cc:39: m_winSize = winSize <= 64 ?
static_cast<uint8_t>(winSize) : 64;
On 2017/12/22 22:28:21, S. Deronne wrote:
> then why not simply changing input parameter to uint8_t if this is sufficient?

changing the parameter moves the cast to the callers.  changed m_winSize to
uint16_t.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
File src/wifi/model/block-ack-manager.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/block-ack-...
src/wifi/model/block-ack-manager.cc:411: 
On 2017/12/22 22:28:21, S. Deronne wrote:
> I do not like it that much... Maybe we could use something like this:
> 
> {
>     uint8_t tid = hdr.GetQosTid ();
>     ...
> }
> 
> {
>     uint8_t tid = hdr.GetQosTid ();
>     ...
> }

That would create a hidden variable error on the tid function parameter.  I have
changed to what I think is more meaningful names.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/ideal-wifi...
src/wifi/model/ideal-wifi-manager.cc:307: selectedNss =
static_cast<uint8_t>(station->m_nss);
On 2017/12/22 22:28:21, S. Deronne wrote:
> m_nss should be moved to an uint8_t

updated as noted.  also updated comment in header file.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:69: {
On 2017/12/22 22:28:21, S. Deronne wrote:
> Then I suggest to add it anyway, for keeping our code consistent.

modified as noted.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:2547: 
On 2017/12/22 22:28:21, S. Deronne wrote:
> I want to find another alternative
> 

moved variable declaration and separated assignment.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
File src/wifi/model/mpdu-standard-aggregator.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/model/mpdu-stand...
src/wifi/model/mpdu-standard-aggregator.cc:85: currentHdr.SetLength
(static_cast<uint8_t>(packet->GetSize ()));
On 2017/12/22 22:28:21, S. Deronne wrote:
> It has just be moved, so still valid.

this source file is no longer part of the wifi module, it was removed from wifi.

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:277: input.interval = MicroSeconds
(static_cast<uint64_t>(delay));
On 2017/12/23 10:19:58, S. Deronne wrote:
> why?

changed variable declaration and removed cast

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/wifi-ma...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/examples/wifi-ma...
src/wifi/examples/wifi-manager-example.cc:319: steps =
static_cast<uint32_t>(std::abs ((int) (clientSelectedStandard.m_snrHigh -
clientSelectedStandard.m_snrLow ) / stepSize) + 1);
On 2017/12/23 10:19:58, S. Deronne wrote:
> I see two cast here, one c++ and one old C style cast. Can't we have something
> better?

change to c++ style.

https://codereview.appspot.com/333070043/diff/80001/src/wifi/helper/wifi-help...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/helper/wifi-help...
src/wifi/helper/wifi-helper.cc:507: header.SetAmpduStatus
(static_cast<uint32_t>(aMpdu.mpduRefNumber), ampduStatusFlags, hdr.GetCrc ());
On 2017/12/23 10:19:58, S. Deronne wrote:
> mpduRefNumber should be limited to uint32_t

changed declaration to uint32_t.  Also changed class members to match.

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
File src/wifi/test/tx-duration-test.cc (left):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
src/wifi/test/tx-duration-test.cc:109: Time calculatedDuration =
phy->GetPayloadDuration (size, txVector, testedFrequency);
On 2017/12/23 10:19:59, S. Deronne wrote:
> testedFrequency should be uint16_t

changed as noted

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
File src/wifi/test/tx-duration-test.cc (right):

https://codereview.appspot.com/333070043/diff/80001/src/wifi/test/tx-duration...
src/wifi/test/tx-duration-test.cc:95: txVector.SetChannelWidth
(static_cast<uint8_t>(channelWidth));
On 2017/12/23 10:19:58, S. Deronne wrote:
> channelWidth should be uint8_t

changed as noted.

[S. Deronne, 2018-02-08 20:53:31]

Robert, am I really reviewing the latest patch set? I'd like to start to merge
with ns-3-dev, but I have the impression this is still using the old way of
doing (before decisions were taken).

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:251: uint64_t delay = 0;
//microseconds
Is this needed? This reduces the granularity.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:272: #if 0
Why? Is this causing issues? Then it might simply be removed.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:157: uint16_t t_clientNss = 1;
why this renaming?

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:192: clientNss =
static_cast<uint8_t>(t_clientNss);
I thought there were a patch to enable the parsing of uint8_t, or am I wrong?

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:429: wifiPhyPtrClient->SetChannelWidth
(static_cast<uint8_t>(clientSelectedStandard.m_width));
I am sure we could avoid all those casts from the last mail from Tom. Is this
really the latest patch set?

[ammo6818-vandals.uidaho.edu, 2018-02-11 19:41:06]

The review comments have been replied to or changes uploaded to address.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:251: uint64_t delay = 0;
//microseconds
On 2018/02/08 20:53:31, S. Deronne wrote:
> Is this needed? This reduces the granularity.

this is what is getting done automatically in the current implementation.  the
double value is getting automatically type cast to uint64_t as part of the
function call so this doesn't change the actual behavior

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:272: #if 0
On 2018/02/08 20:53:31, S. Deronne wrote:
> Why? Is this causing issues? Then it might simply be removed.

I have changed all of the examples and tests to not log output by default.  this
standardizes the implementation across all modules (some were already this way
and some were not) and it helps with verification testing time (test.py).  If a
developer needs to see the log output, they can uncomment these lines.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:157: uint16_t t_clientNss = 1;
On 2018/02/08 20:53:31, S. Deronne wrote:
> why this renaming?

see the next comment.  this is because the existing implementation does not
handle int8_t and unit8_t as numeric values, rather as ascii characters.  
because of that, you either need to create temporary int16_t / uint16_t
variables of put type casts throughout.  this change is part of the changes
necessary to eliminate the need or type casts.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:192: clientNss =
static_cast<uint8_t>(t_clientNss);
On 2018/02/08 20:53:31, S. Deronne wrote:
> I thought there were a patch to enable the parsing of uint8_t, or am I wrong?

the current implementation (which I have not changed) treats int8_t / uint8_t as
char.  What that causes is the value that the application receives is the ascii
character equivalent not the numeric value.  There has not been an update to the
current implementation since it might break any functions that do expect ascii
characters. 

the logging code did get updated so that int8_t and uint8_t logged values appear
as the numeric value not the ascii character

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:429: wifiPhyPtrClient->SetChannelWidth
(static_cast<uint8_t>(clientSelectedStandard.m_width));
On 2018/02/08 20:53:31, S. Deronne wrote:
> I am sure we could avoid all those casts from the last mail from Tom. Is this
> really the latest patch set?

updated to add additional changes so the type casts can  be removed.

[S. Deronne, 2018-02-16 21:24:33]

I will start pushing what can be done already.
My main concerns are still those NS-UNUSED that I think we should not not use,
and there are for sure some casts that can be avoided.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:251: uint64_t delay = 0;
//microseconds
I do not see such cast in the example, do you mean this gets casted by
CommandLine?

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:272: #if 0
I don't think this is complex to manually add those lines if a developer wants
to investigate an issue.
I suggest not to use such if defines.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:192: clientNss =
static_cast<uint8_t>(t_clientNss);
So we should push those logging code changes asap and this work should be based
on this fix, so no need to cast here anymore. Right?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
src/wifi/helper/athstats-helper.cc:226: NS_UNUSED(txPower);
I though Tom suggested not to used NS_UNUSED? I would instead add those to the
logging function below.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
src/wifi/helper/athstats-helper.cc:232: AthstatsWifiTraceSink::PhyStateTrace
(std::string context, Time start, Time duration, WifiPhy::State state)
OK to push this change

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
File src/wifi/helper/athstats-helper.h (left):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
src/wifi/helper/athstats-helper.h:203: * Open a file for output
OK to push changes to this file in ns-3-dev

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:204: uint64_t rate = 0;
correct!

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:208: header.SetRate (static_cast<uint8_t>(rate));
I guess we don't have another solution here? :-(

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:398: uint64_t rate = 0;
correct!

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:580: NS_UNUSED(promiscuous);
to be avoided

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:433: Ptr<DcaTxopTest> dca =
CreateObject<DcaTxopTest> (this, static_cast<uint32_t>(m_dcfStates.size ()));
No way to avoid this???

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/spectrum-w...
File src/wifi/test/spectrum-wifi-phy-test.cc (left):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/spectrum-w...
src/wifi/test/spectrum-wifi-phy-test.cc:224: {
We should definitely agree whether we use NS_UNUSED or not... I am not in favor
to use this.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
File src/wifi/test/tx-duration-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:109: Time calculatedDuration =
phy->GetPayloadDuration (size, txVector,
static_cast<uint16_t>(testedFrequency));
testedFrequency is already uint16_t in latest ns-3-dev, so this cast can be
removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:116: << " datarate=" <<
payloadMode.GetDataRate (static_cast<uint8_t>(channelWidth), guardInterval, 1)
channelWidth is already uint8_t in latest ns-3-dev, so this cast can be removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:126: calculatedDuration =
phy->GetPayloadDuration (size, txVector,
static_cast<uint16_t>(testedFrequency));
testedFrequency is already uint16_t in latest ns-3-dev, so this cast can be
removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:134: << " datarate=" <<
payloadMode.GetDataRate (static_cast<uint8_t>(channelWidth), guardInterval, 1)
channelWidth is already uint8_t in latest ns-3-dev, so this cast can be removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:150: txVector.SetChannelWidth
(static_cast<uint8_t>(channelWidth));
channelWidth is already uint8_t in latest ns-3-dev, so this cast can be removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:164: Time calculatedDuration =
phy->CalculateTxDuration (size, txVector,
static_cast<uint16_t>(testedFrequency));
testedFrequency is already uint16_t in latest ns-3-dev, so this cast can be
removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:182: calculatedDuration =
phy->CalculateTxDuration (size, txVector,
static_cast<uint16_t>(testedFrequency));
testedFrequency is already uint16_t in latest ns-3-dev, so this cast can be
removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-error...
File src/wifi/test/wifi-error-rate-models-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-error...
src/wifi/test/wifi-error-rate-models-test.cc:69: uint64_t size = (1024 + 40 +
14) * 8;
OK to push this to ns-3-dev already

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1223: uint32_t numBands =
static_cast<uint32_t>(c->GetNumBands ());
Or change GetNumBands to return uint32_t???

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1324: Simulator::Schedule (Seconds (0.5),
&Bug2483TestCase::SendPacketBurst, this, static_cast<uint8_t>(5), apDevice.Get
(0), staDevice.Get (0)->GetAddress ());
why? Or change SendPacketBurst signature.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1335: uint8_t numberTuples =
static_cast<uint8_t>(m_distinctTuples.size ());
can you avoid this cast in any way?

[S. Deronne, 2018-02-17 08:13:16]

I am starting to push some changes to ns-3-dev, please address my comments so
that I can proceed further.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aarf-wifi...
File src/wifi/model/aarf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aarf-wifi...
src/wifi/model/aarf-wifi-manager.cc:248: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aarf-wifi...
src/wifi/model/aarf-wifi-manager.cc:279: rtsTxVector = WifiTxVector (mode,
GetDefaultTxPowerLevel (), static_cast<uint8_t>(GetLongRetryCount (station)),
GetPreambleForTransmission (mode, GetAddress (station)), 800, 1, 1, 0,
static_cast<uint8_t>(channelWidth), GetAggregation (station), false);
other solution: GetLongRetryCount could return uint8_t? If you anyway think this
could overpass 255, then we should adapt WifiTxVector.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aarfcd-wi...
File src/wifi/model/aarfcd-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aarfcd-wi...
src/wifi/model/aarfcd-wifi-manager.cc:317: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/amrr-wifi...
File src/wifi/model/amrr-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/amrr-wifi...
src/wifi/model/amrr-wifi-manager.cc:356: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aparf-wif...
File src/wifi/model/aparf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aparf-wif...
src/wifi/model/aparf-wifi-manager.cc:131: m_maxPower =
static_cast<uint32_t>(phy->GetTxPowerEnd ());
those power could be uint8_t then?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/aparf-wif...
src/wifi/model/aparf-wifi-manager.cc:332: DataRate rate = DataRate
(mode.GetDataRate (static_cast<uint8_t>(channelWidth)));
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/arf-wifi-...
File src/wifi/model/arf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/arf-wifi-...
src/wifi/model/arf-wifi-manager.cc:223: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/cara-wifi...
File src/wifi/model/cara-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/cara-wifi...
src/wifi/model/cara-wifi-manager.cc:194: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:37: uint8_t m_nss;             //!< number
of spacial streams
Correct :-)

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:126: txVector.SetChannelWidth
(static_cast<uint8_t>(j));
I think we already discussed this case... I don't know the conclusion at that
time, i know the problem is that the width go to 160 MHz then it gets doubles to
320 which overpass 255. We could do a while loop and when it gets 160 at the
end, we break. Agree with this approach?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:152: guardInterval =
static_cast<uint16_t>(GetPhy ()->GetGuardInterval ().GetNanoSeconds ());
guard interval could be defined as int64 to avoid such cast?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:158: " channel width " <<
static_cast<uint16_t> (k) <<
Fix logging to avoid uint8_t interpretation as char?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:223: NS_UNUSED(txMode);
NO...

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:308: selectedNss =
static_cast<uint8_t>(station->m_nss);
station->m_nss is already uint8_t, correct?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:325: guardInterval =
static_cast<uint16_t>(std::max (GetShortGuardInterval (station) ? 400 : 800,
GetPhy ()->GetShortGuardInterval () ? 400 : 800));
mmmh, a trick to avoid this cast?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:271: uint32_t groupId = GetVhtGroupId
(streams, sgi, static_cast<uint8_t>(chWidth));
chWidth should be defined as uint8_t, same suggestion as for ideal wifi manager
to fix the looping over the supported channel widths.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:880: station->m_sampleWait =
static_cast<uint32_t>(16 + 2 * station->m_avgAmpduLen);
needed because of m_avgAmpduLen? then change it to uint32_t.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1317:
static_cast<uint32_t>(m_ewmaLevel));
needed because of m_ewmaLevel? then change it to uint32_t.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:366: if (m_currentRate !=
mode.GetDataRate (static_cast<uint8_t>(channelWidth)) && !station->m_isSampling)
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:429: uint32_t idx;
I don't get this, you declare it now as uint32_t, and later you cast to uint8_t
what is passed to it. idx should not be more than uint8_t though...

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:991: uint8_t numSampleRates =
static_cast<uint8_t>(station->m_nModes);
m_nModes should be changed to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:1076: float tmpTh = rate.throughput /
100000.0f;
OK

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/onoe-wifi...
File src/wifi/model/onoe-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/onoe-wifi...
src/wifi/model/onoe-wifi-manager.cc:307: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/parf-wifi...
File src/wifi/model/parf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/parf-wifi...
src/wifi/model/parf-wifi-manager.cc:98: m_minPower =
static_cast<uint32_t>(phy->GetTxPowerStart ());
m_minPower should be uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/parf-wifi...
src/wifi/model/parf-wifi-manager.cc:304: DataRate rate = DataRate
(mode.GetDataRate (static_cast<uint8_t>(channelWidth)));
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:253: th.m_ewnd = static_cast<uint32_t>(ceil
(m_tau / totalTxTime.GetSeconds ()));
change type of m_ewnd?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:347: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
File src/wifi/model/rrpaa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:245: station->m_nRate =
static_cast<uint8_t>(GetNSupported (station));
GetNSupported should return uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:307: th.m_ewnd = static_cast<uint32_t>(ceil
(m_tau / totalTxTime.GetSeconds ()));
change type of m_ewnd?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:395: DataRate rate = DataRate
(mode.GetDataRate (static_cast<uint8_t>(channelWidth)));
Instead, I will change the channel width to uint8_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
File src/wifi/model/rrpaa-wifi-manager.h (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.h:223: uint8_t m_nPowerLevels;      //!<
Number of power levels.
OK

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-util...
File src/wifi/model/wifi-utils.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-util...
src/wifi/model/wifi-utils.cc:86: gi =
static_cast<uint16_t>(heGuardInterval.GetNanoSeconds ());
a better solution: declare gi as int64_t?

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-utils.h
File src/wifi/model/wifi-utils.h (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-util...
src/wifi/model/wifi-utils.h:93: bool Is5Ghz (double frequency);
duplicates???

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:127: return
static_cast<uint32_t>(m_phyList.size ());
other idea to avoid this cast?

[S. Deronne, 2018-02-17 20:00:22]

I pushed some changes to ns-3-dev already. Could you please update your patchset
and address previous comments?
How can I test those warnings on my machine?

[ammo6818-vandals.uidaho.edu, 2018-02-18 05:13:10]

Updated patch for latest changes uploaded.

https://codereview.appspot.com/333070043/diff/40001/src/wifi/examples/test-in...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/40001/src/wifi/examples/test-in...
src/wifi/examples/test-interference-helper.cc:121: txVector.SetTxPowerLevel
(static_cast<int8_t>(m_input.txPowerLevelA));
On 2017/12/17 15:39:14, S. Deronne wrote:
> why int8_t? I think it is uint8_t

cast eliminated

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:251: uint64_t delay = 0;
//microseconds
On 2018/02/16 21:24:31, S. Deronne wrote:
> I do not see such cast in the example, do you mean this gets casted by
> CommandLine? 

because the function being called expects a uint64_t value, the double value is
getting converted automatically to a unit64_t as part of the function call by
the compiler on Linux.  The flags under visual studio disable that automatic
down convert and produce an error.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:272: #if 0
On 2018/02/16 21:24:31, S. Deronne wrote:
> I don't think this is complex to manually add those lines if a developer wants
> to investigate an issue.
> I suggest not to use such if defines.

removed.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:192: clientNss =
static_cast<uint8_t>(t_clientNss);
On 2018/02/16 21:24:31, S. Deronne wrote:
> So we should push those logging code changes asap and this work should be
based
> on this fix, so no need to cast here anymore. Right?
> 

the changes you are referring to are outputting uint8_t values in log output
statements.

in this case, the AddValue function doesn't know how to deal with the unit8_t
type.  Is the uint8_t type an ascii character or does the ascii character
represent a numeric value?  For example should the text '0' return the ascii
value of the character 0 or should it return the value of 0?

The whole reason for these changes was to try and eliminate casts, but it
appears this implementation might be more confusing than the casts so I am going
to revert back and add the casts.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:429: wifiPhyPtrClient->SetChannelWidth
(static_cast<uint8_t>(clientSelectedStandard.m_width));
On 2018/02/11 19:41:06, ammo6818-vandals.uidaho.edu wrote:
> On 2018/02/08 20:53:31, S. Deronne wrote:
> > I am sure we could avoid all those casts from the last mail from Tom. Is
this
> > really the latest patch set?
> 
> updated to add additional changes so the type casts can  be removed.

removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/athstats...
src/wifi/helper/athstats-helper.cc:226: NS_UNUSED(txPower);
On 2018/02/16 21:24:31, S. Deronne wrote:
> I though Tom suggested not to used NS_UNUSED? I would instead add those to the
> logging function below.

changed as noted.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:208: header.SetRate (static_cast<uint8_t>(rate));
On 2018/02/16 21:24:31, S. Deronne wrote:
> I guess we don't have another solution here? :-(

any other change would just create more temporary variables which I don't think
is better.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:580: NS_UNUSED(promiscuous);
On 2018/02/16 21:24:31, S. Deronne wrote:
> to be avoided

i understood the requirement from Tom to minimize these if at all possible.  the
idea (as I understand it) was to use these warnings initially to identify areas
where the code can be updated to eliminate the warning by use NS_UNUSED where
there is no other choice.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/parf-wifi...
File src/wifi/model/parf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/parf-wifi...
src/wifi/model/parf-wifi-manager.cc:98: m_minPower =
static_cast<uint32_t>(phy->GetTxPowerStart ());
On 2018/02/17 08:13:15, S. Deronne wrote:
> m_minPower should be uint8_t

updated as noted.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:347: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
On 2018/02/17 08:13:15, S. Deronne wrote:
> Instead, I will change the channel width to uint8_t

change incorporated.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
File src/wifi/model/rrpaa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:245: station->m_nRate =
static_cast<uint8_t>(GetNSupported (station));
On 2018/02/17 08:13:15, S. Deronne wrote:
> GetNSupported should return uint8_t

cast removed.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:307: th.m_ewnd = static_cast<uint32_t>(ceil
(m_tau / totalTxTime.GetSeconds ()));
On 2018/02/17 08:13:15, S. Deronne wrote:
> change type of m_ewnd?

that member variable is passed as a parameter into multiple functions which
would cause multiple other changes

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:395: DataRate rate = DataRate
(mode.GetDataRate (static_cast<uint8_t>(channelWidth)));
On 2018/02/17 08:13:15, S. Deronne wrote:
> Instead, I will change the channel width to uint8_t

your changes incorporated.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-utils.h
File src/wifi/model/wifi-utils.h (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/wifi-util...
src/wifi/model/wifi-utils.h:93: bool Is5Ghz (double frequency);
On 2018/02/17 08:13:16, S. Deronne wrote:
> duplicates???

duplicates eliminated

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:127: return
static_cast<uint32_t>(m_phyList.size ());
On 2018/02/17 08:13:16, S. Deronne wrote:
> other idea to avoid this cast?

size() evaluates to uint32_t when built as 32-bit and uint64_t when built as
64-bit.  Since this is a virtual function changing the return type requires a
significant number of changes.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:433: Ptr<DcaTxopTest> dca =
CreateObject<DcaTxopTest> (this, static_cast<uint32_t>(m_dcfStates.size ()));
On 2018/02/16 21:24:31, S. Deronne wrote:
> No way to avoid this???

made other code changes too eliminate

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/spectrum-w...
File src/wifi/test/spectrum-wifi-phy-test.cc (left):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/spectrum-w...
src/wifi/test/spectrum-wifi-phy-test.cc:224: {
On 2018/02/16 21:24:32, S. Deronne wrote:
> We should definitely agree whether we use NS_UNUSED or not... I am not in
favor
> to use this.

removed

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
File src/wifi/test/tx-duration-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:109: Time calculatedDuration =
phy->GetPayloadDuration (size, txVector,
static_cast<uint16_t>(testedFrequency));
On 2018/02/16 21:24:32, S. Deronne wrote:
> testedFrequency is already uint16_t in latest ns-3-dev, so this cast can be
> removed.

removed all static casts in this file

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1223: uint32_t numBands =
static_cast<uint32_t>(c->GetNumBands ());
On 2018/02/16 21:24:32, S. Deronne wrote:
> Or change GetNumBands to return uint32_t???

changed variable declaration to size_t

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1324: Simulator::Schedule (Seconds (0.5),
&Bug2483TestCase::SendPacketBurst, this, static_cast<uint8_t>(5), apDevice.Get
(0), staDevice.Get (0)->GetAddress ());
On 2018/02/16 21:24:32, S. Deronne wrote:
> why? Or change SendPacketBurst signature.

changed signature

https://codereview.appspot.com/333070043/diff/180001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1335: uint8_t numberTuples =
static_cast<uint8_t>(m_distinctTuples.size ());
changed declaration

[S. Deronne, 2018-02-18 07:44:41]

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:251: uint64_t delay = 0;
//microseconds
OK, thanks.

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (right):

https://codereview.appspot.com/333070043/diff/140001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:192: clientNss =
static_cast<uint8_t>(t_clientNss);
OK

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:208: header.SetRate (static_cast<uint8_t>(rate));
OK then.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/helper/wifi-hel...
src/wifi/helper/wifi-helper.cc:580: NS_UNUSED(promiscuous);
I think by adding debug logs at the beginning of those functions containing all
input parameters would eliminate the need for those NS_UNUSED.

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/180001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:127: return
static_cast<uint32_t>(m_phyList.size ());
OK, I guess we have to accept this cast then.

[S. Deronne, 2018-02-18 08:06:44]

https://codereview.appspot.com/333070043/diff/200001/src/wifi/examples/test-i...
File src/wifi/examples/test-interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/examples/test-i...
src/wifi/examples/test-interference-helper.cc:275: //LogComponentEnable
("SimpleFrameCaptureModel", LOG_LEVEL_ALL);
I suggest to simply remove them

https://codereview.appspot.com/333070043/diff/200001/src/wifi/examples/wifi-m...
File src/wifi/examples/wifi-manager-example.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/examples/wifi-m...
src/wifi/examples/wifi-manager-example.cc:106: StandardInfo (std::string name,
WifiPhyStandard standard, uint16_t width, double snrLow, double snrHigh, double
xMin, double xMax, double yMax)
widht could be uint8_t in serverSelectedStandard?

https://codereview.appspot.com/333070043/diff/200001/src/wifi/helper/athstats...
File src/wifi/helper/athstats-helper.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/helper/athstats...
src/wifi/helper/athstats-helper.cc:236: void
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:89: DcaTxopTest (DcfManagerTest *test,
std::size_t i);
If I understand this size_t, we could also use it for functions using size()
functions instead of the cast, for example in the one in yanswifiphy using
m_phyList.size (). Or do I misunderstand this use of size_t?

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/spectrum-w...
File src/wifi/test/spectrum-wifi-phy-test.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/spectrum-w...
src/wifi/test/spectrum-wifi-phy-test.cc:224: {
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/tx-duratio...
File src/wifi/test/tx-duration-test.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/test/tx-duratio...
src/wifi/test/tx-duration-test.cc:169: << " channelWidth=" << channelWidth
This change requires the logging patch to be pushed?

[S. Deronne, 2018-02-18 14:52:57]

I will continue to push. 
I still do not agree with all those NS_UNUSED.
Please always check against the latest ns-3-dev so that I can see the remaining
part, as long as I do not know how to reproduce on my machine, I do not know the
remaining warnings (or whether additional ones popped up)...

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/aarf-wifi...
File src/wifi/model/aarf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/aarf-wifi...
src/wifi/model/aarf-wifi-manager.cc:248: if (m_currentRate != mode.GetDataRate
(static_cast<uint8_t>(channelWidth)))
channelWidth is already uint8_t

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/aparf-wif...
File src/wifi/model/aparf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/aparf-wif...
src/wifi/model/aparf-wifi-manager.cc:130: m_minPower =
static_cast<uint8_t>(phy->GetTxPowerStart ());
m_minPower is already uint8_t

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-cache.h (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
src/wifi/model/block-ack-cache.h:80: uint16_t m_winSize; ///< window size
OK

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:373: || SwitchToBlockAckIfNeeded (recip,
tid, hdr.GetSequenceNumber ())))
why this rename? If not applicable for cleaning up warning, please remove from
the patchset.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:410: Mac48Address hdrRecipient =
hdr.GetAddr1 ();
idem

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:570:
static_cast<uint8_t>((*queueIt).hdr.GetFragmentNumber ())))
GetFragmentNumber will return uint8_t in a next commit.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:633: m_stationManager->ReportAmpduTxStatus
(recipient, tid, static_cast<uint8_t>(nSuccessfulMpdus),
static_cast<uint8_t>(nFailedMpdus), rxSnr, dataSnr);
nSuccessfulMpdus and nFailedMpdus will be declared as uint8_t in a next commit.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ctrl-head...
File src/wifi/model/ctrl-headers.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ctrl-head...
src/wifi/model/ctrl-headers.cc:702: index = static_cast<uint8_t>(4096 -
m_startingSeq + seq);
to make a decision: or we keep those case, or we accept to use uint16_t for
index.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-erro...
File src/wifi/model/dsss-error-rate-model.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-erro...
src/wifi/model/dsss-error-rate-model.cc:98: #ifdef HAVE_GSL
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-erro...
File src/wifi/model/dsss-error-rate-model.h (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-erro...
src/wifi/model/dsss-error-rate-model.h:117: static double SymbolErrorProb256Cck
(double e1); /// equation (17) in Pursley's paper
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-para...
File src/wifi/model/dsss-parameter-set.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/dsss-para...
src/wifi/model/dsss-parameter-set.cc:82: }
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/erp-infor...
File src/wifi/model/erp-information.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/erp-infor...
src/wifi/model/erp-information.cc:112: }
OK to push those changes

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/extended-...
File src/wifi/model/extended-capabilities.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/extended-...
src/wifi/model/extended-capabilities.cc:414: NS_UNUSED(extendedcapabilities);
maybe a better solution is to implement the body of those functions?

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/he-capabi...
File src/wifi/model/he-capabilities.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/he-capabi...
src/wifi/model/he-capabilities.cc:22: #include "ns3/unused.h"
to be removed

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ht-operat...
File src/wifi/model/ht-operation.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ht-operat...
src/wifi/model/ht-operation.cc:335: uint8_t val = 0;
correct

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ht-operat...
src/wifi/model/ht-operation.cc:371: m_reservedInformationSubset2_1 =
static_cast<uint8_t>((ctrl >> 5) & 0x07ff);
OK

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ht-operat...
src/wifi/model/ht-operation.cc:481: uint16_t informationsubset3 = i.ReadU16 ();
correct

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:37: uint8_t m_nss;             //!< number
of spacial streams
Correct :-)

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:221: NS_LOG_FUNCTION (this << station <<
rxSnr <<txMode);
I prefer this solution :-)

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/interfere...
File src/wifi/model/interference-helper.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/interfere...
src/wifi/model/interference-helper.cc:130: m_firstPower (0.0),
does this really change something? I had myself cleaned those 0.0, but if you
say this plays a role then at least I know :-)

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/interfere...
src/wifi/model/interference-helper.cc:305: double csr =
m_errorRateModel->GetChunkSuccessRate (mode, txVector, snir, (uint32_t)nbits);
C-style cast NOK, and this functions expects a uint64_t and not uint32_t, so
this cast is not valid.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1957: uint16_t mappedSeqControl =
static_cast<uint16_t>(QosUtilsMapSeqControlToUniqueInteger
(hdr.GetSequenceControl (), endSequence));
make QosUtilsMapSeqControlToUniqueInteger returns uint16_t?

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:2417: if (!edcaIt->second->GetMpduAggregator
()->CanBeAggregated (peekedPacket->GetSize () + peekedHdr.GetSize () +
WIFI_MAC_FCS_LENGTH, aggregatedPacket, static_cast<uint8_t>(size)))
blockack size should be adapted in the code to be limited to uint8_t, an BACK
will not be more than 255 bytes.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:2682: ampdutag.SetRemainingNbOfMpdus
(static_cast<uint8_t>(i - 1));
define i as uint8_t should be fine

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mac-low.h...
src/wifi/model/mac-low.h:661: void SendDataAfterCts (Time duration);
well spotted :-)

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mgt-heade...
File src/wifi/model/mgt-headers.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mgt-heade...
src/wifi/model/mgt-headers.cc:441: i.WriteHtolsbU16
(static_cast<uint16_t>(m_beaconInterval / 1024));
I guess we do not have choice here, so OK for this cast.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/mgt-heade...
src/wifi/model/mgt-headers.cc:977: m_actionValue =
static_cast<uint8_t>(action.selfProtectedAction);
I also think only cast is the solution here

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/regular-w...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/regular-w...
src/wifi/model/regular-wifi-mac.cc:1368: GetVOQueue ()->GetMpduAggregator
()->SetMaxAmpduSize (static_cast<uint16_t>(m_voMaxAmpduSize));
m_voMaxAmpduSize should be uint16_t,  as well as be, bk and vi, and both msdu
and mpdu.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:208: station->m_nRate =
static_cast<uint8_t>(GetNSupported (station));
GetNSupported returns uint8_t now

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/spectrum-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/spectrum-...
src/wifi/model/spectrum-wifi-phy.cc:110: m_rxSpectrumModel =
WifiSpectrumValueHelper::GetSpectrumModel (GetFrequency (), GetChannelWidth (),
GetBandBandwidth (), static_cast<uint8_t>(GetGuardBandwidth ()));
Make GetGuardBandwidth as uint8_t? But I fear this could be larger than 255, so
maybe a better solution is to change GetSpectrumModel definition.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/spectrum-...
src/wifi/model/spectrum-wifi-phy.cc:381: uint32_t guardBandwidth = 0;
correct

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/supported...
File src/wifi/model/supported-rates.h (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/supported...
src/wifi/model/supported-rates.h:157: void AddSupportedRate (uint64_t bs);
not sure this is allowed, to be check in the IE definition from the standard.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/vht-capab...
File src/wifi/model/vht-capabilities.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/vht-capab...
src/wifi/model/vht-capabilities.cc:115: uint32_t vhtinfo = i.ReadLsbtohU32 ();
Good

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/vht-capab...
src/wifi/model/vht-capabilities.cc:184: n = (i * 2) + 32;
Indeed, not needed to redefine  n :-)

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/vht-capab...
File src/wifi/model/vht-capabilities.h (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/vht-capab...
src/wifi/model/vht-capabilities.h:290: uint8_t m_supportedChannelWidthSet; ///<
supported channel width set
OK, correct.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-info...
File src/wifi/model/wifi-information-element-vector.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-info...
src/wifi/model/wifi-information-element-vector.cc:108: case 0: // eliminate
compiler warning
OK

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-mac-...
File src/wifi/model/wifi-mac-header.h (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-mac-...
src/wifi/model/wifi-mac-header.h:591: uint8_t m_qosStuff; ///< QOS stuff
OK to push this change

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-mac-...
File src/wifi/model/wifi-mac-queue.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-mac-...
src/wifi/model/wifi-mac-queue.cc:100: TimeValue (MilliSeconds (500)),
OK

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.h:1962: bool     m_stbc;                  //!< Flag if
STBC is used
Very good, I will push.

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-radi...
File src/wifi/model/wifi-radio-energy-model.cc (right):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-radi...
src/wifi/model/wifi-radio-energy-model.cc:470: NS_LOG_FUNCTION (this);
not needed

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-remo...
File src/wifi/model/wifi-remote-station-manager.cc (left):

https://codereview.appspot.com/333070043/diff/200001/src/wifi/model/wifi-remo...
src/wifi/model/wifi-remote-station-manager.cc:27: #include "wifi-phy.h"
OK

[ammo6818-vandals.uidaho.edu, 2018-03-05 23:17:08]

I have uploaded an updated patch to incorporate all the module changes and to
fix coding style issues.

Please let me know if you have any comments.

[ammo6818-vandals.uidaho.edu, 2018-03-14 02:41:23]

Patch has been updated with latest module changes.

[S. Deronne, 2018-03-27 10:53:35]

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (left):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:390: {
are those size_t needed because they are filled in with .size()? Are they all
needed?
For a future correct coding, when should we use size_t and when not?

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1255: Bug2483TestCase::SendPacketBurst (unsigned int
numPackets, Ptr<NetDevice> sourceDevice,
why unsigned int?

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1322: Simulator::Schedule (Seconds (0.5),
&Bug2483TestCase::SendPacketBurst, this, 5U, apDevice.Get (0), staDevice.Get
(0)->GetAddress ());
mainly for my understanding, why is this U needed?

[ammo6818-vandals.uidaho.edu, 2018-03-30 03:57:29]

Patch has been updated with remaining files for the patch.

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (left):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:390: {
On 2018/03/27 10:53:35, S. Deronne wrote:
> are those size_t needed because they are filled in with .size()? Are they all
> needed?
> For a future correct coding, when should we use size_t and when not?

.size evaluates to a uint32_t when built as 32-bit and uint64_t when built as
64-bit.  To be consistent .size and std::size_t should be used together and
.size not mixed with uint32_ or uint64_t

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1255: Bug2483TestCase::SendPacketBurst (unsigned int
numPackets, Ptr<NetDevice> sourceDevice,
On 2018/03/27 10:53:35, S. Deronne wrote:
> why unsigned int?

i is declared as uint16_t.  mixing uint16_t and int causes a signed / unsigned
mismatch warning.  changing the parameter from int to unsigned int eliminates
that warning.

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1322: Simulator::Schedule (Seconds (0.5),
&Bug2483TestCase::SendPacketBurst, this, 5U, apDevice.Get (0), staDevice.Get
(0)->GetAddress ());
On 2018/03/27 10:53:35, S. Deronne wrote:
> mainly for my understanding, why is this U needed?

constants are defined as int by default.  Because of the previous issue, this
parameter needs to be unsigned int.  Adding the U suffix changes the constant
from int to unsigned int to match the previous change.

[S. Deronne, 2018-03-30 06:29:51]

Thanks for the quick update.
I am mainly doubting about all casts, I will still try to see if alternatives
are possible.

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
File src/wifi/test/dcf-manager-test.cc (left):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/dcf-manage...
src/wifi/test/dcf-manager-test.cc:390: {
OK thanks.

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1255: Bug2483TestCase::SendPacketBurst (unsigned int
numPackets, Ptr<NetDevice> sourceDevice,
since i is changed to uint16_t, why not declaring numPackets as uint16_t??

https://codereview.appspot.com/333070043/diff/280001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1322: Simulator::Schedule (Seconds (0.5),
&Bug2483TestCase::SendPacketBurst, this, 5U, apDevice.Get (0), staDevice.Get
(0)->GetAddress ());
OK, clear.

[S. Deronne, 2018-03-30 06:56:55]

Here are some additional comments. I cannot investigate myself right now since
the setup is being repaired by Tom and I have no setup locally.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:412: AgreementsI i = m_agreements.find
(std::make_pair (hdrRecipient, hdrTid));
I guess the problem here is that we have two variables with the same name?
Then I suggest to rename only one of them.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ctrl-head...
File src/wifi/model/ctrl-headers.cc (left):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ctrl-head...
src/wifi/model/ctrl-headers.cc:688: }
not better to have IndexInBitmap function to return uint16_t?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
File src/wifi/model/dcf-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
src/wifi/model/dcf-manager.cc:175: m_slotTimeUs = static_cast<uint32_t>
(slotTime.GetMicroSeconds ());
m_slotTimeUs could be int64_t?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
src/wifi/model/dcf-manager.cc:450: uint32_t nus = static_cast<uint32_t>
((Simulator::Now () - backoffStart).GetMicroSeconds ());
same comment here for nus

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
File src/wifi/model/edca-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
src/wifi/model/edca-parameter-set.cc:76: uint8_t ECWmin = static_cast<uint8_t>
(log2 (cwMin + 1));
do you also have a warning if ECWmin is not defined as uint8_t but as the type
returned by log2?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:128: txVector.SetChannelWidth
(static_cast<uint8_t> (j));
I will have a look today how to change j to uint8_t.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:491: guardInterval = static_cast<uint16_t>
(std::max (GetShortGuardInterval (station) ? 400 : 800, GetPhy
()->GetShortGuardInterval () ? 400 : 800));
not better to have another type for guardInterval to avoid all those casts?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:499: uint8_t sentMpdus = static_cast<uint8_t>
(m_aggregateQueue[GetTid (packet, *hdr)]->GetNPackets ());
another type for sentMpdus?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1239: uint8_t queueSize = static_cast<uint8_t>
(m_aggregateQueue[GetTid (packet, *hdr)]->GetNPackets ());
another type for queueSize?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1374: uint8_t nTxMpdus = static_cast<uint8_t>
(m_aggregateQueue[tid]->GetNPackets ());
another type for nTxMpdus?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1860: uint16_t mappedSeqControl =
static_cast<uint16_t> (QosUtilsMapSeqControlToUniqueInteger
(hdr.GetSequenceControl (), endSequence));
another type for mappedSeqControl?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1943: uint16_t mappedStart = static_cast<uint16_t>
(QosUtilsMapSeqControlToUniqueInteger (seq, endSequence));
another type for mappedStart?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1236: NS_LOG_DEBUG (i << " " <<
GetMcsSupported (station,  static_cast<uint8_t>
(station->m_groupsTable[j].m_ratesTable[i].mcsIndex)) <<
My bad, I will change mcsIndex to uint8_t, I had forgotten that one in previous
rework.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1261: static_cast<uint32_t>
(m_ewmaLevel));
change fct signature of move m_ewmaLevel as uint32_t should be possible. I
cannot give a try right now since I do not have a setup for the time being.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:253: th.m_ewnd = static_cast<uint32_t> (ceil
(m_tau / totalTxTime.GetSeconds ()));
change type of m_ewnd?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rrpaa-wif...
File src/wifi/model/rrpaa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:300: th.m_ewnd = static_cast<uint32_t>
(ceil (m_tau / totalTxTime.GetSeconds ()));
change type of m_ewnd?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode.cc
File src/wifi/model/wifi-mode.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:152: uint32_t numberOfBitsPerSubcarrier =
static_cast<uint32_t> (log2 (GetConstellationSize ()));
change type of numberOfBitsPerSubcarrier?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:789: uint32_t uid = static_cast<uint32_t>
(m_itemList.size ());
change type of uid to size_t?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
File src/wifi/model/wifi-net-device.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
src/wifi/model/wifi-net-device.cc:499: return static_cast<uint8_t>
(QosUtilsMapTidToAc (priority));
change return type?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.cc:1332: return static_cast<uint8_t>
(m_bssMembershipSelectorSet.size ());
return a size_t?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-remo...
File src/wifi/model/wifi-remote-station-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-remo...
src/wifi/model/wifi-remote-station-manager.cc:1904: return static_cast<uint8_t>
(m_bssBasicRateSet.size ());
return a size_t is not better?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-util...
File src/wifi/model/wifi-utils.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-util...
src/wifi/model/wifi-utils.cc:86: gi =
static_cast<uint16_t>(heGuardInterval.GetNanoSeconds ());
define gi as int64_t? More globally we should do that for every time variable we
keep in microseconds, so that we avoid all those casts.
What do you think?

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:129: return static_cast<uint32_t>
(m_phyList.size ());
can't we change to return a size_t?

[S. Deronne, 2018-04-03 20:34:11]

Robert, could you update with the latest ns-3-dev and address my comments?
Thanks.

[ammo6818-vandals.uidaho.edu, 2018-04-05 17:51:02]

Updated patch for review comments and latest module changes.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:412: AgreementsI i = m_agreements.find
(std::make_pair (hdrRecipient, hdrTid));
On 2018/03/30 06:56:53, S. Deronne wrote:
> I guess the problem here is that we have two variables with the same name?
> Then I suggest to rename only one of them.

both tid and recipient are variables in an outer code block so both have to be
renamed.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ctrl-head...
File src/wifi/model/ctrl-headers.cc (left):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ctrl-head...
src/wifi/model/ctrl-headers.cc:688: }
On 2018/03/30 06:56:53, S. Deronne wrote:
> not better to have IndexInBitmap function to return uint16_t?

changed as noted.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
File src/wifi/model/dcf-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
src/wifi/model/dcf-manager.cc:175: m_slotTimeUs = static_cast<uint32_t>
(slotTime.GetMicroSeconds ());
On 2018/03/30 06:56:53, S. Deronne wrote:
> m_slotTimeUs could be int64_t?

no, changing causes issues with UpdateBackoffSlotsNow()

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
src/wifi/model/dcf-manager.cc:450: uint32_t nus = static_cast<uint32_t>
((Simulator::Now () - backoffStart).GetMicroSeconds ());
On 2018/03/30 06:56:53, S. Deronne wrote:
> same comment here for nus

no, changing causes issues with UpdateBackoffSlotsNow()

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
File src/wifi/model/edca-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
src/wifi/model/edca-parameter-set.cc:76: uint8_t ECWmin = static_cast<uint8_t>
(log2 (cwMin + 1));
On 2018/03/30 06:56:53, S. Deronne wrote:
> do you also have a warning if ECWmin is not defined as uint8_t but as the type
> returned by log2?

no, but the native type for log2 is double and the next statement that bit
shifts ECWmin would not make sense.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:491: guardInterval = static_cast<uint16_t>
(std::max (GetShortGuardInterval (station) ? 400 : 800, GetPhy
()->GetShortGuardInterval () ? 400 : 800));
On 2018/03/30 06:56:53, S. Deronne wrote:
> not better to have another type for guardInterval to avoid all those casts?

no, because it is passed into multiple functions as a parameter and those
functions would have to change.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:499: uint8_t sentMpdus = static_cast<uint8_t>
(m_aggregateQueue[GetTid (packet, *hdr)]->GetNPackets ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> another type for sentMpdus?

changed to unit32_t

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1239: uint8_t queueSize = static_cast<uint8_t>
(m_aggregateQueue[GetTid (packet, *hdr)]->GetNPackets ());
On 2018/03/30 06:56:53, S. Deronne wrote:
> another type for queueSize?

no its passed in to other functions which compare to a member variable that is
uint8_t

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1374: uint8_t nTxMpdus = static_cast<uint8_t>
(m_aggregateQueue[tid]->GetNPackets ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> another type for nTxMpdus?

no its passed in to other functions which compare to a member variable that is
uint8_t

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1860: uint16_t mappedSeqControl =
static_cast<uint16_t> (QosUtilsMapSeqControlToUniqueInteger
(hdr.GetSequenceControl (), endSequence));
On 2018/03/30 06:56:54, S. Deronne wrote:
> another type for mappedSeqControl?

changed to uint32_t

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1943: uint16_t mappedStart = static_cast<uint16_t>
(QosUtilsMapSeqControlToUniqueInteger (seq, endSequence));
On 2018/03/30 06:56:54, S. Deronne wrote:
> another type for mappedStart?

changed to uint32_t

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1261: static_cast<uint32_t>
(m_ewmaLevel));
On 2018/03/30 06:56:54, S. Deronne wrote:
> change fct signature of move m_ewmaLevel as uint32_t should be possible. I
> cannot give a try right now since I do not have a setup for the time being.

changed called function parameter to double

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:253: th.m_ewnd = static_cast<uint32_t> (ceil
(m_tau / totalTxTime.GetSeconds ()));
no, it causes other static casts to be created so there is no improvement

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rrpaa-wif...
File src/wifi/model/rrpaa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rrpaa-wif...
src/wifi/model/rrpaa-wifi-manager.cc:300: th.m_ewnd = static_cast<uint32_t>
(ceil (m_tau / totalTxTime.GetSeconds ()));
On 2018/03/30 06:56:54, S. Deronne wrote:
> change type of m_ewnd?

no, it causes other static casts to be required so there is no improvement.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode.cc
File src/wifi/model/wifi-mode.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:152: uint32_t numberOfBitsPerSubcarrier =
static_cast<uint32_t> (log2 (GetConstellationSize ()));
On 2018/03/30 06:56:54, S. Deronne wrote:
> change type of numberOfBitsPerSubcarrier?

no, i believe changing it to double may change the result of the function

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:789: uint32_t uid = static_cast<uint32_t>
(m_itemList.size ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> change type of uid to size_t?

No, create other static cast so there is no difference

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
File src/wifi/model/wifi-net-device.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
src/wifi/model/wifi-net-device.cc:499: return static_cast<uint8_t>
(QosUtilsMapTidToAc (priority));
On 2018/03/30 06:56:54, S. Deronne wrote:
> change return type?

no, the returned value is a non base type (user defined type)which would cause a
lot of other changes.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.cc:1332: return static_cast<uint8_t>
(m_bssMembershipSelectorSet.size ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> return a size_t?

no, this would move static casts to lots of called functions so there is no
benefit

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-remo...
File src/wifi/model/wifi-remote-station-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-remo...
src/wifi/model/wifi-remote-station-manager.cc:1904: return static_cast<uint8_t>
(m_bssBasicRateSet.size ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> return a size_t is not better?

no, this would create lots of static casts is calling functions so there is no
savings.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-util...
File src/wifi/model/wifi-utils.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-util...
src/wifi/model/wifi-utils.cc:86: gi =
static_cast<uint16_t>(heGuardInterval.GetNanoSeconds ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> define gi as int64_t? More globally we should do that for every time variable
we
> keep in microseconds, so that we avoid all those casts.
> What do you think?

no, gi is used in lots of places as uint16_t so it would create more casts than
it saves.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:129: return static_cast<uint32_t>
(m_phyList.size ());
On 2018/03/30 06:56:54, S. Deronne wrote:
> can't we change to return a size_t?

no, this is a virtual function in the network module that is used extensively
elsewhere

[S. Deronne, 2018-04-05 19:46:50]

Thanks for the update.
I am still convinced a lot of casts can be avoided, but of course this need more
work/changes, but IMO we need to avoid casts as much as we can. I am willing to
help, but I need a setup.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:412: AgreementsI i = m_agreements.find
(std::make_pair (hdrRecipient, hdrTid));
On 2018/04/05 17:51:00, ammo6818-vandals.uidaho.edu wrote:
> On 2018/03/30 06:56:53, S. Deronne wrote:
> > I guess the problem here is that we have two variables with the same name?
> > Then I suggest to rename only one of them.
> 
> both tid and recipient are variables in an outer code block so both have to be
> renamed.

Ok, there is indeed already tid in the function signature

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
File src/wifi/model/dcf-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/dcf-manag...
src/wifi/model/dcf-manager.cc:175: m_slotTimeUs = static_cast<uint32_t>
(slotTime.GetMicroSeconds ());
Which kind of issue? This maybe worth to see how to fix the issue, we need to
avoid cast as much as possible.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
File src/wifi/model/edca-parameter-set.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/edca-para...
src/wifi/model/edca-parameter-set.cc:76: uint8_t ECWmin = static_cast<uint8_t>
(log2 (cwMin + 1));
On 2018/04/05 17:51:00, ammo6818-vandals.uidaho.edu wrote:
> On 2018/03/30 06:56:53, S. Deronne wrote:
> > do you also have a warning if ECWmin is not defined as uint8_t but as the
type
> > returned by log2?
> 
> no, but the native type for log2 is double and the next statement that bit
> shifts ECWmin would not make sense.

correct, then I think this cast cannot be avoided

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:491: guardInterval = static_cast<uint16_t>
(std::max (GetShortGuardInterval (station) ? 400 : 800, GetPhy
()->GetShortGuardInterval () ? 400 : 800));
I do not see a problem in changing functions signatures if this is to avoid a
cast :-)

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:253: th.m_ewnd = static_cast<uint32_t> (ceil
(m_tau / totalTxTime.GetSeconds ()));
where? maybe multiple change are needed. I would definitely need a setup to give
a try.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode.cc
File src/wifi/model/wifi-mode.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:152: uint32_t numberOfBitsPerSubcarrier =
static_cast<uint32_t> (log2 (GetConstellationSize ()));
no, not if you correct round up (or down) as would do a cast, but round up/down
will return the correct type.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
File src/wifi/model/wifi-net-device.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/wifi-net-...
src/wifi/model/wifi-net-device.cc:499: return static_cast<uint8_t>
(QosUtilsMapTidToAc (priority));
I think it worth a try.

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/300001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:129: return static_cast<uint32_t>
(m_phyList.size ());
Then we can change it in several places, i guess you have the same issue in
other submodules then.

[ammo6818-vandals.uidaho.edu, 2018-05-03 02:52:47]

Patch has been updated with latest module changes.

[S. Deronne, 2018-05-03 19:09:18]

Hi,

Thanks, but could you also update the wireless examples?
We are closer to a conclusion there.

Kind regards,
Sébastien.

> Le 3 mai 2018 à 04:52, womenrgr8id@gmail.com a écrit :
> 
> Patch has been updated with latest module changes.
> 
> https://codereview.appspot.com/333070043/

[S. Deronne, 2018-05-03 21:47:23]

https://codereview.appspot.com/333070043/diff/340001/src/wifi/examples/wifi-p...
File src/wifi/examples/wifi-phy-configuration.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/examples/wifi-p...
src/wifi/examples/wifi-phy-configuration.cc:23: #include "ns3/wifi-module.h"
?

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:415: AgreementsI i = m_agreements.find
(std::make_pair (hdrRecipient, hdrTid));
all those recipient params are not needed, I have a patch I will push.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/ctrl-head...
File src/wifi/model/ctrl-headers.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/ctrl-head...
src/wifi/model/ctrl-headers.cc:680: uint16_t index;
OK, this is better than a cast.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:258:
m_minstrelGroups[groupId].chWidth = channelWidth;
why? channel width is now defined at uint16_t, why down casting to uint8_t?

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1603:
MinstrelHtWifiManager::CalculateEwmsd (double oldEwmsd, double currentProb,
double ewmaProb, double weight)
why double?

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1735: txTime = GetFirstMpduTxTime
(groupId, GetMcsSupported (station, static_cast<uint8_t>
(station->m_groupsTable[groupId].m_ratesTable[i].mcsIndex)));
why? mcsIndex is already uint8_t

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:370: WifiMode mode = GetSupported
(station, static_cast<uint8_t> (station->m_txrate));
m_txrate should be uint8_t, I will change

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:436: int delta = static_cast<int>
((station->m_totalPacketsCount * m_lookAroundRate / 100)) -
(station->m_samplePacketsCount + station->m_numSamplesDeferred / 2);
I suggest to change delta type.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:463: NS_LOG_DEBUG ("Sample rate = " <<
idx << "(" << GetSupported (station, static_cast<uint8_t> (idx)) << ")");
I changed idx to uint8_t, I will push in ns-3-dev asap.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:694: ")\tmax prob=" << index_max_prob <<
"(" << GetSupported (station, static_cast<uint8_t> (index_max_prob)) << ")");
I will update all the rates to uint8_t, please re-test once this is pushed to
mainstream.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:409: double ploss = (station->m_nFailed /
thresholds.m_ewnd);
this is strange, are you sure? I fear we will not get a double here.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/spectrum-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/spectrum-...
src/wifi/model/spectrum-wifi-phy.cc:337: Ptr<SpectrumValue> txPowerSpectrum =
GetTxPowerSpectralDensity (static_cast<uint16_t>
(GetCenterFrequencyForChannelWidth (txVector)), txVector.GetChannelWidth (),
txPowerWatts, txVector.GetMode ().GetModulationClass ());
GetCenterFrequencyForChannelWidth should return uint16_t

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.cc:1035: NS_LOG_FUNCTION (this << frequency << width);
good catch

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.h:499: typedef std::pair<uint16_t, uint16_t>
FrequencyWidthPair;
correct

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.h:1596: uint8_t FindChannelNumberForFrequencyWidth
(uint16_t frequency, uint16_t width) const;
correct

https://codereview.appspot.com/333070043/diff/340001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1256: for (uint16_t i = 0; i < numPackets; i++)
I still dont get this, you use unsigned int in signature and here you convert to
uint16_t

[ammo6818-vandals.uidaho.edu, 2018-05-07 15:05:55]

Updates to address latest review comments have been uploaded.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/examples/wifi-p...
File src/wifi/examples/wifi-phy-configuration.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/examples/wifi-p...
src/wifi/examples/wifi-phy-configuration.cc:23: #include "ns3/wifi-module.h"
On 2018/05/03 21:47:21, S. Deronne wrote:
> ?

Covered in a separate E-mail about this not included in wscript

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/block-ack...
File src/wifi/model/block-ack-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/block-ack...
src/wifi/model/block-ack-manager.cc:415: AgreementsI i = m_agreements.find
(std::make_pair (hdrRecipient, hdrTid));
On 2018/05/03 21:47:21, S. Deronne wrote:
> all those recipient params are not needed, I have a patch I will push.

Ok, I will ignore this file for now.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:258:
m_minstrelGroups[groupId].chWidth = channelWidth;
On 2018/05/03 21:47:22, S. Deronne wrote:
> why? channel width is now defined at uint16_t, why down casting to uint8_t?

Updated for latest module changes.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1603:
MinstrelHtWifiManager::CalculateEwmsd (double oldEwmsd, double currentProb,
double ewmaProb, double weight)
On 2018/05/03 21:47:21, S. Deronne wrote:
> why double?

This is called with a parameter that is a double.  Matches the instance where
this is called from.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1735: txTime = GetFirstMpduTxTime
(groupId, GetMcsSupported (station, static_cast<uint8_t>
(station->m_groupsTable[groupId].m_ratesTable[i].mcsIndex)));
On 2018/05/03 21:47:22, S. Deronne wrote:
> why? mcsIndex is already uint8_t

removed.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:370: WifiMode mode = GetSupported
(station, static_cast<uint8_t> (station->m_txrate));
On 2018/05/03 21:47:22, S. Deronne wrote:
> m_txrate should be uint8_t, I will change

ok, i will ignore

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:436: int delta = static_cast<int>
((station->m_totalPacketsCount * m_lookAroundRate / 100)) -
(station->m_samplePacketsCount + station->m_numSamplesDeferred / 2);
On 2018/05/03 21:47:22, S. Deronne wrote:
> I suggest to change delta type. 

That will cause other casts later on.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:463: NS_LOG_DEBUG ("Sample rate = " <<
idx << "(" << GetSupported (station, static_cast<uint8_t> (idx)) << ")");
On 2018/05/03 21:47:22, S. Deronne wrote:
> I changed idx to uint8_t, I will push in ns-3-dev asap.

Ok, I will ignore

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-wifi-manager.cc:694: ")\tmax prob=" << index_max_prob <<
"(" << GetSupported (station, static_cast<uint8_t> (index_max_prob)) << ")");
On 2018/05/03 21:47:22, S. Deronne wrote:
> I will update all the rates to uint8_t, please re-test once this is pushed to
> mainstream.

OK

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/rraa-wifi...
File src/wifi/model/rraa-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/rraa-wifi...
src/wifi/model/rraa-wifi-manager.cc:409: double ploss = (station->m_nFailed /
thresholds.m_ewnd);
On 2018/05/03 21:47:22, S. Deronne wrote:
> this is strange, are you sure? I fear we will not get a double here.

The result will be automatically cast to a double.

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/spectrum-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/spectrum-...
src/wifi/model/spectrum-wifi-phy.cc:337: Ptr<SpectrumValue> txPowerSpectrum =
GetTxPowerSpectralDensity (static_cast<uint16_t>
(GetCenterFrequencyForChannelWidth (txVector)), txVector.GetChannelWidth (),
txPowerWatts, txVector.GetMode ().GetModulationClass ());
On 2018/05/03 21:47:22, S. Deronne wrote:
> GetCenterFrequencyForChannelWidth should return uint16_t

changed to uint16_t

https://codereview.appspot.com/333070043/diff/340001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1256: for (uint16_t i = 0; i < numPackets; i++)
On 2018/05/03 21:47:22, S. Deronne wrote:
> I still dont get this, you use unsigned int in signature and here you convert
to
> uint16_t

made consistent

[S. Deronne, 2018-05-11 14:46:06]

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/340001/src/wifi/model/minstrel-...
src/wifi/model/minstrel-ht-wifi-manager.cc:1603:
MinstrelHtWifiManager::CalculateEwmsd (double oldEwmsd, double currentProb,
double ewmaProb, double weight)
On 2018/05/07 15:05:54, ammo6818-vandals.uidaho.edu wrote:
> On 2018/05/03 21:47:21, S. Deronne wrote:
> > why double?
> 
> This is called with a parameter that is a double.  Matches the instance where
> this is called from.

OK, I am not sure this is correct to have it double in the code, but this needs
to be checked later with Matias. In the meantime, I will do that change.

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/ap-wifi-m...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/ap-wifi-m...
src/wifi/model/ap-wifi-mac.cc:478: edcaParameters.SetBeAifsn
(static_cast<uint8_t> (edca->GetAifsn ()));
maybe worth to check in the standard whether we can limit aifsn to uint8_t

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/aparf-wif...
File src/wifi/model/aparf-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/aparf-wif...
src/wifi/model/aparf-wifi-manager.cc:164: station->m_prevRateIndex =
static_cast<uint8_t> (station->m_nSupported - 1);
I will change m_nSupported to uint8_t

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/ideal-wif...
File src/wifi/model/ideal-wifi-manager.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/ideal-wif...
src/wifi/model/ideal-wifi-manager.cc:128: txVector.SetChannelWidth
(static_cast<uint8_t> (j));
SetChannelWidth expects uint16_t

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:522: for (uint32_t i = 0; i < sentMpdus; i++)
why?

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1248: uint8_t queueSize = static_cast<uint8_t>
(m_aggregateQueue[GetTid (packet, *hdr)]->GetNPackets ());
I suggest to change type of queueSize

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:1383: uint8_t nTxMpdus = static_cast<uint8_t>
(m_aggregateQueue[tid]->GetNPackets ());
I am facing a dilemma: or we change type of nTxMpdus (+ the needed functions),
or we accept casts. What is the preferred solution of other reviewers?

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:2329: if (!edcaIt->second->GetMpduAggregator
()->CanBeAggregated (peekedPacket->GetSize () + peekedHdr.GetSize () +
WIFI_MAC_FCS_LENGTH, aggregatedPacket, static_cast<uint8_t> (size)))
size could become uint8_t, I'll give a try.

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:2409: blockAckSize = static_cast<uint16_t>
(packet->GetSize () + hdr.GetSize () + WIFI_MAC_FCS_LENGTH);
unavoidable?

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/mac-low.c...
src/wifi/model/mac-low.cc:2594: ampdutag.SetRemainingNbOfMpdus
(static_cast<uint8_t> (i - 1));
I guess we could change i to uint8_t

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/spectrum-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/spectrum-...
src/wifi/model/spectrum-wifi-phy.cc:337: Ptr<SpectrumValue> txPowerSpectrum =
GetTxPowerSpectralDensity (static_cast<uint16_t>
(GetCenterFrequencyForChannelWidth (txVector)), txVector.GetChannelWidth (),
txPowerWatts, txVector.GetMode ().GetModulationClass ());
GetCenterFrequencyForChannelWidth already return uint16_t => cast not needed

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/supported...
File src/wifi/model/supported-rates.cc (left):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/supported...
src/wifi/model/supported-rates.cc:152: if ((bs & 0x7f) ==
BSS_MEMBERSHIP_SELECTOR_HT_PHY
I see some functions still using uint32_t, like GetRate, I feel some
inconsistencies here...

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-mode.cc
File src/wifi/model/wifi-mode.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-mode...
src/wifi/model/wifi-mode.cc:165: uint32_t numberOfBitsPerSubcarrier =
static_cast<uint32_t> (log2 (GetConstellationSize ()));
an alternative is to declare it double, but it physically has no sense, so I am
here in favor or a cast. I am note sure though uint32_t is realistic, I think we
can reduce this to uint16_t.

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-phy-...
File src/wifi/model/wifi-phy-tag.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-tag.cc:50: i.WriteU16 (static_cast<uint16_t>
(m_mpduType));
OK, no other choice also here

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-radi...
File src/wifi/model/wifi-radio-energy-model.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-radi...
src/wifi/model/wifi-radio-energy-model.cc:320: remainingTime = NanoSeconds
(static_cast<uint64_t> (1e9 * (remainingEnergy / (m_sleepCurrentA *
supplyVoltage))));
indeed, if it expects uint64_t, I think we should keep those casts. The other
solution to use everywhere uint64_t for the variables is not a correct solution
to me, so I do not see other alternatives.

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-remo...
File src/wifi/model/wifi-remote-station-manager.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/wifi-remo...
src/wifi/model/wifi-remote-station-manager.cc:1931: return static_cast<uint8_t>
(m_bssBasicMcsSet.size ());
I tend to agree with this cast

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/yans-wifi...
File src/wifi/model/yans-wifi-channel.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/model/yans-wifi...
src/wifi/model/yans-wifi-channel.cc:134: return static_cast<uint32_t>
(m_phyList.size ());
I suggest to return size_t, and to adapt all base classes.

https://codereview.appspot.com/333070043/diff/360001/src/wifi/test/wifi-test.cc
File src/wifi/test/wifi-test.cc (right):

https://codereview.appspot.com/333070043/diff/360001/src/wifi/test/wifi-test....
src/wifi/test/wifi-test.cc:1256: for (unsigned int i = 0; i < numPackets; i++)
I do not understand why we need this change, we even do not use the i later on
in the for loop.