Code review - Issue 338840043: Eliminate Visual Studio compiler warningshttps://codereview.appspot.com/2018-08-22T11:35:30+00:00rietveld
Message from unknown
2017-11-10T01:15:25+00:00ammo6818-vandals.uidaho.eduurn:md5:f07ed4a6c91d6f3626818dd32aab85b1
Message from unknown
2017-11-12T01:51:38+00:00ammo6818-vandals.uidaho.eduurn:md5:62a95ea2bdab2b9ea5a486c04a410be7
Message from unknown
2017-12-09T01:10:34+00:00ammo6818-vandals.uidaho.eduurn:md5:a012f266bf32666f5a2add8983c6fe26
Message from unknown
2017-12-16T10:40:05+00:00ammo6818-vandals.uidaho.eduurn:md5:459f95900f8d9fd035c491022a62d721
Message from unknown
2017-12-31T16:12:56+00:00ammo6818-vandals.uidaho.eduurn:md5:70318c7bebccb1819bd62fe821b42e3b
Message from unknown
2018-01-15T01:25:13+00:00ammo6818-vandals.uidaho.eduurn:md5:92ea1c31bbc81f73d292bf0299dbbde1
Message from unknown
2018-03-07T22:24:56+00:00ammo6818-vandals.uidaho.eduurn:md5:47a0b25d59ebd02340213983a03d0b3f
Message from unknown
2018-03-15T00:09:41+00:00ammo6818-vandals.uidaho.eduurn:md5:223ba32a3b99abfec031c5917b4c9680
Message from womenrgr8id@gmail.com
2018-03-15T00:12:22+00:00ammo6818-vandals.uidaho.eduurn:md5:65860f997f190c0ddd4f6b9d23298689
Patch updated with latest module changes
Message from Biljana.Bojovic@gmail.com
2018-08-22T11:35:30+00:00Biljana Bojovićurn:md5:bdbb07332d233d32a5600691748fb90e
Hi Robert,
please find below my comments. You did a great work! There are some minor details that in my opinion should be changed.
General comments are:
1. I prefer to change variable type over static casts in code
2. When changing type, the variable that represents the same thing over different parts of the code, e.g. bandwidth should be always of the same size, e.g. uint8_t
3. Instead of making silent NS_ASSERTs I would convert it to analog NS_ABBORT.
If you have any doubt regarding comments please do not hesitate to contact me.
Best regards,
Biljana
https://codereview.appspot.com/338840043/diff/160001/src/lte/helper/point-to-point-epc-helper.cc
File src/lte/helper/point-to-point-epc-helper.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/helper/point-to-point-epc-helper.cc#newcode92
src/lte/helper/point-to-point-epc-helper.cc:92: (void)retval; // make compiler happy
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/helper/radio-environment-map-helper.h
File src/lte/helper/radio-environment-map-helper.h (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/helper/radio-environment-map-helper.h#newcode64
src/lte/helper/radio-environment-map-helper.h:64: uint16_t GetBandwidth () const;
I would try to stick with uint8_t, so that everywhere in the code is consistent.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/cqa-ff-mac-scheduler.cc
File src/lte/model/cqa-ff-mac-scheduler.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/cqa-ff-mac-scheduler.cc#newcode943
src/lte/model/cqa-ff-mac-scheduler.cc:943: uint8_t rbgId = static_cast<uint8_t> ((dciRbg.at (dciRbg.size () - 1) + 1) % numberOfRBGs);
I think that we should change type of elemtn of vector dciRbg to be uint8_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-enb-application.cc
File src/lte/model/epc-enb-application.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-enb-application.cc#newcode270
src/lte/model/epc-enb-application.cc:270: (void)found; // make compiler happy
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-enb-application.cc#newcode331
src/lte/model/epc-enb-application.cc:331: (void)sentBytes; // make compiler happy
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-tft-classifier.cc
File src/lte/model/epc-tft-classifier.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-tft-classifier.cc#oldcode197
src/lte/model/epc-tft-classifier.cc:197:
If possible I would prefer to change type of variable then performing cast
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-x2-header.cc
File src/lte/model/epc-x2-header.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-x2-header.cc#oldcode1138
src/lte/model/epc-x2-header.cc:1138: std::vector <EpcX2Sap::CellInformationItem>::size_type sz = m_cellInformationList.size ();
In general I prefer using word if it is not too long than the abbreviation. In this case I would put "size" intead of "sz".
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-x2.cc
File src/lte/model/epc-x2.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/epc-x2.cc#oldcode755
src/lte/model/epc-x2.cc:755: NS_LOG_INFO ("GTP-U header: " << gtpu);
Again, if possible I would prefer that we change the size of length then to perform cast. I see it as more clean solution.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.cc
File src/lte/model/lte-amc.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.cc#oldcode306
src/lte/model/lte-amc.cc:306: int
uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.cc
File src/lte/model/lte-amc.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.cc#newcode294
src/lte/model/lte-amc.cc:294: uint16_t
uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.h
File src/lte/model/lte-amc.h (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-amc.h#newcode84
src/lte/model/lte-amc.h:84: uint16_t GetDlTbSizeFromMcs (int mcs, int nprb);
Please check if this is really necessary? I think that it should be at least uint32_t to allow the maximum TB size by the LTE specification. The function comment says that the TB is in bits, so I think that it should be 32...
https://www.etsi.org/deliver/etsi_ts/136200_136299/136213/12.03.00_60/ts_136213v120300p.pdf
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-mac.cc
File src/lte/model/lte-enb-mac.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-mac.cc#newcode1142
src/lte/model/lte-enb-mac.cc:1142: rar.rapId = static_cast<uint8_t> (itRapId->second);
Please, double check if this conversion from 32 to 8 is correct.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-net-device.cc
File src/lte/model/lte-enb-net-device.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-net-device.cc#oldcode329
src/lte/model/lte-enb-net-device.cc:329: LteEnbNetDevice::SetCsgId (uint32_t csgId)
We should change input parameter to uint16_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-enb-rrc.cc#oldcode605
src/lte/model/lte-enb-rrc.cc:605: hpi.asConfig.sourceMasterInformationBlock.dlBandwidth = m_rrc->m_dlBandwidth;
Change type of m_ulBandwidth and m_dlBandwidth to uint8_t to have consistent definitions and usage.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ffr-distributed-algorithm.cc
File src/lte/model/lte-ffr-distributed-algorithm.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ffr-distributed-algorithm.cc#newcode490
src/lte/model/lte-ffr-distributed-algorithm.cc:490: uint16_t rbgSize = static_cast<uint16_t> (GetRbgSize (m_dlBandwidth));
Change rbgSize to correct type instead of performing cast.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ffr-soft-algorithm.cc
File src/lte/model/lte-ffr-soft-algorithm.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ffr-soft-algorithm.cc#newcode343
src/lte/model/lte-ffr-soft-algorithm.cc:343: i < static_cast<uint8_t> ((m_dlCommonSubBandwidth + m_dlEdgeSubBandOffset + m_dlEdgeSubBandwidth) / rbgSize); i++)
If possible change all bandwidth types to uint8_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-fr-strict-algorithm.cc
File src/lte/model/lte-fr-strict-algorithm.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-fr-strict-algorithm.cc#oldcode318
src/lte/model/lte-fr-strict-algorithm.cc:318: + m_dlEdgeSubBandwidth / rbgSize); i++)
change bandwidth type instead of cast
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-rlc-am.cc
File src/lte/model/lte-rlc-am.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-rlc-am.cc#newcode1660
src/lte/model/lte-rlc-am.cc:1660: (void)firstVrMs; // make compier happy
I would change NS_ASSERT_MSG to NS_ABORT_MSG_UNLESS so this line would be available in all builds, so I do not think that compiler would complain.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-spectrum-value-helper.cc
File src/lte/model/lte-spectrum-value-helper.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-spectrum-value-helper.cc#newcode212
src/lte/model/lte-spectrum-value-helper.cc:212: uint16_t bandwidth; ///< bandwidth
We should definitively decide and be consistent which size will have bandwidth. This seems that is currently limited by RRC serialization messages, which assume that the bandwidth is 8 bit integer. So, I would try to stick to this.
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ue-mac.cc
File src/lte/model/lte-ue-mac.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/lte-ue-mac.cc#newcode530
src/lte/model/lte-ue-mac.cc:530: (void)prachMask; // make compiler happy
Again, I would change this to NS_ABORT_MSG_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/no-op-component-carrier-manager.cc
File src/lte/model/no-op-component-carrier-manager.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/model/no-op-component-carrier-manager.cc#newcode476
src/lte/model/no-op-component-carrier-manager.cc:476: (void)componentCarrierId; // make compiler happy
NS_ABORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pathloss-model.cc
File src/lte/test/lte-test-pathloss-model.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pathloss-model.cc#oldcode163
src/lte/test/lte-test-pathloss-model.cc:163: }
I prefer that we change the parameter of LtePathlossModelStystemTestCase to uint8_ instead of performing conversion
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pf-ff-mac-scheduler.cc
File src/lte/test/lte-test-pf-ff-mac-scheduler.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pf-ff-mac-scheduler.cc#newcode374
src/lte/test/lte-test-pf-ff-mac-scheduler.cc:374: : TestCase (BuildNameString (static_cast<uint16_t> (dist.size ()), dist)),
I prefer that we change the type of BuildNameString to uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pss-ff-mac-scheduler.cc
File src/lte/test/lte-test-pss-ff-mac-scheduler.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-pss-ff-mac-scheduler.cc#newcode498
src/lte/test/lte-test-pss-ff-mac-scheduler.cc:498: m_nUser (static_cast<uint16_t> (dist.size ())),
I would rather change type of m_nUser to uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-tdbet-ff-mac-scheduler.cc
File src/lte/test/lte-test-tdbet-ff-mac-scheduler.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-tdbet-ff-mac-scheduler.cc#newcode359
src/lte/test/lte-test-tdbet-ff-mac-scheduler.cc:359: m_nUser (static_cast<uint16_t> (dist.size ())),
m_nUser to uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-tdtbfq-ff-mac-scheduler.cc
File src/lte/test/lte-test-tdtbfq-ff-mac-scheduler.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-tdtbfq-ff-mac-scheduler.cc#newcode497
src/lte/test/lte-test-tdtbfq-ff-mac-scheduler.cc:497: m_nUser (static_cast<uint16_t> (dist.size ())),
m_nUser to uint32_t
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc
File src/lte/test/lte-test-ue-measurements.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc#oldcode671
src/lte/test/lte-test-ue-measurements.cc:671: NS_ASSERT (rnti == 1);
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc#oldcode673
src/lte/test/lte-test-ue-measurements.cc:673:
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc#oldcode1261
src/lte/test/lte-test-ue-measurements.cc:1261: NS_ASSERT (rnti == 1);
NS_ABORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc#oldcode1262
src/lte/test/lte-test-ue-measurements.cc:1262: NS_ASSERT (cellId == 1);
NS_ABBORT_UNLESS
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/lte-test-ue-measurements.cc#oldcode1666
src/lte/test/lte-test-ue-measurements.cc:1666:
Can size be changed to uint32_t to avoid so many static casts?
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/test-lte-rrc.cc
File src/lte/test/test-lte-rrc.cc (right):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/test-lte-rrc.cc#newcode443
src/lte/test/test-lte-rrc.cc:443: uint8_t ueDlEarfcn = static_cast<uint8_t> (ueRrc->GetDlEarfcn ());
Is it possible to define here ueDlEarfcn with the correct uint type to avoid cast?
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/test-lte-x2-handover-measures.cc
File src/lte/test/test-lte-x2-handover-measures.cc (left):
https://codereview.appspot.com/338840043/diff/160001/src/lte/test/test-lte-x2-handover-measures.cc#oldcode595
src/lte/test/test-lte-x2-handover-measures.cc:595: uint8_t ueUlEarfcn = ueRrc->GetUlEarfcn ();
Again, is it possible to declare these variable with the correct uint type?