Code review - Issue 336970043: Eliminate Visual Studio compiler warningshttps://codereview.appspot.com/2018-05-16T15:06:29+00:00rietveld
Message from unknown
2017-11-09T06:13:58+00:00ammo6818-vandals.uidaho.eduurn:md5:394b03b47d37ec2668a3058743055b7d
Message from unknown
2017-11-12T01:49:28+00:00ammo6818-vandals.uidaho.eduurn:md5:7b6edf6ef5a88cb51c09045d483ffd33
Message from unknown
2017-12-09T00:37:03+00:00ammo6818-vandals.uidaho.eduurn:md5:f2a36f7e18a983099ffb5b32494323df
Message from unknown
2017-12-27T21:04:30+00:00ammo6818-vandals.uidaho.eduurn:md5:fb7125a99029f77d61fb00bc1c062e41
Message from unknown
2017-12-31T15:34:38+00:00ammo6818-vandals.uidaho.eduurn:md5:d7a23c1140cbf3016f1fb852c6d5a2ef
Message from unknown
2018-01-15T00:36:06+00:00ammo6818-vandals.uidaho.eduurn:md5:9f9fb53ef076312389f88b67fc280208
Message from unknown
2018-01-29T02:40:12+00:00ammo6818-vandals.uidaho.eduurn:md5:669edd950bd3d61975483c63044feb8c
Message from unknown
2018-02-11T19:32:27+00:00ammo6818-vandals.uidaho.eduurn:md5:5826ef390b8209e33524b5598068c8c1
Message from unknown
2018-02-18T00:54:09+00:00ammo6818-vandals.uidaho.eduurn:md5:70f2407906b76487f6fb13117dcc60c6
Message from womenrgr8id@gmail.com
2018-02-18T05:44:41+00:00ammo6818-vandals.uidaho.eduurn:md5:cf1c23506d5d79b9fe331276b9de63ab
Please let me know if there are any other review comments or merge into ns-3-dev
Message from tomh.org@gmail.com
2018-02-20T02:19:28+00:00Tom Hendersonurn:md5:302e4876ad037b2e027beb6ec0c5b406
Once
https://codereview.appspot.com/336970043/diff/220001/src/core/examples/sample-log-time-format.cc
File src/core/examples/sample-log-time-format.cc (right):
https://codereview.appspot.com/336970043/diff/220001/src/core/examples/sample-log-time-format.cc#newcode127
src/core/examples/sample-log-time-format.cc:127: #if 0
The point of this example is to display the log output. If we need to suppress it, add a command line argument 'quiet' (default boolean false) and if set to true, suppress the log component enabling.
https://codereview.appspot.com/336970043/diff/220001/src/core/model/log.cc
File src/core/model/log.cc (right):
https://codereview.appspot.com/336970043/diff/220001/src/core/model/log.cc#newcode689
src/core/model/log.cc:689: m_os << static_cast<int16_t>(param);
I would echo Peter's question here: why is the ", " missing (here and below)?
Message from womenrgr8id@gmail.com
2018-02-20T04:40:39+00:00ammo6818-vandals.uidaho.eduurn:md5:605af0e3b0aed91f8c39bcec5a9e7cd0
Update posted to address latest review comments
https://codereview.appspot.com/336970043/diff/220001/src/core/examples/sample-log-time-format.cc
File src/core/examples/sample-log-time-format.cc (right):
https://codereview.appspot.com/336970043/diff/220001/src/core/examples/sample-log-time-format.cc#newcode127
src/core/examples/sample-log-time-format.cc:127: #if 0
On 2018/02/20 02:19:28, Tom Henderson wrote:
> The point of this example is to display the log output. If we need to suppress
> it, add a command line argument 'quiet' (default boolean false) and if set to
> true, suppress the log component enabling.
restored original implementation
https://codereview.appspot.com/336970043/diff/220001/src/core/model/log.cc
File src/core/model/log.cc (right):
https://codereview.appspot.com/336970043/diff/220001/src/core/model/log.cc#newcode689
src/core/model/log.cc:689: m_os << static_cast<int16_t>(param);
On 2018/02/20 02:19:28, Tom Henderson wrote:
> I would echo Peter's question here: why is the ", " missing (here and below)?
I matched the implementation found in ParameterLogger::operator<< <std::string>(const std::string param) which doesnt output a comma if this is the first output.
Message from tomh.org@gmail.com
2018-02-20T05:14:23+00:00Tom Hendersonurn:md5:262188272d67f9ef7455c75314200bff
>
> I matched the implementation found in ParameterLogger::operator<<
> <std::string>(const std::string param) which doesnt output a comma if this is
> the first output.
I see now, thanks.
Message from pdbj@mac.com
2018-02-21T23:16:33+00:00Peter Barnesurn:md5:46ef2ed6dcd1a0c603f845f61d177f12
On 2018/02/18 05:44:41, ammo6818-vandals.uidaho.edu wrote:
> Please let me know if there are any other review comments or merge into ns-3-dev
My and Tom's suggested cleanups aren't present in patch set 10 (doxygen, test case logging, various others). Please post an accurate patch set which passes your VS warnings.
Thanks,
Peter
Message from unknown
2018-02-25T12:11:11+00:00ammo6818-vandals.uidaho.eduurn:md5:a87f44c46a3972e6d5d485fdf1921dd5
Message from womenrgr8id@gmail.com
2018-02-25T12:12:16+00:00ammo6818-vandals.uidaho.eduurn:md5:51ea06a14649a38a617ee32bfe22876c
On 2018/02/21 23:16:33, Peter Barnes wrote:
> On 2018/02/18 05:44:41, http://ammo6818-vandals.uidaho.edu wrote:
> > Please let me know if there are any other review comments or merge into
> ns-3-dev
>
> My and Tom's suggested cleanups aren't present in patch set 10 (doxygen, test
> case logging, various others). Please post an accurate patch set which passes
> your VS warnings.
>
> Thanks,
> Peter
My apologizes, the wrong files were uploaded. A corrected patch has been uploaded.
Message from tomh.org@gmail.com
2018-02-25T18:14:57+00:00Tom Hendersonurn:md5:e1a6b3b1b2e5ab8f5b7368fb4b145913
Just some minor things; would like to include this by Tuesday (as two patches-- doxygen and compiler issues)
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/command-line-example.cc
File src/core/examples/command-line-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/command-line-example.cc#newcode86
src/core/examples/command-line-example.cc:86: CommandLine cmd;
can be stripped from final patch (whitespace change)
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc
File src/core/examples/hash-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc#newcode362
src/core/examples/hash-example.cc:362: long double k64 = static_cast<long double> (0xFFFFFFFFFFFFFFFFULL);
I am not sure about these; isn't long double an 8-byte quantity? why is a cast needed on k64 and not k32?
Shouldn't it be something like std::numeric_limits<long double>::max()?
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/main-test-sync.cc
File src/core/examples/main-test-sync.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/main-test-sync.cc#newcode45
src/core/examples/main-test-sync.cc:45: NS_LOG_COMPONENT_DEFINE ("TestSync");
can be stripped from final patch (whitespace change)
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h
File src/core/model/assert.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h#newcode75
src/core/model/assert.h:75: } \
is this just a whitespace change?
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object-base.h
File src/core/model/object-base.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object-base.h#newcode82
src/core/model/object-base.h:82: namespace ns3 {
whitespace change
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object.h
File src/core/model/object.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object.h#newcode668
src/core/model/object.h:668: #endif /* OBJECT_H */
whitespace change
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h
File src/core/model/vector.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode252
src/core/model/vector.h:252: std::istream &operator >> (std::istream &is, Vector2D &vector);
this perhaps can be pulled out of this patch and committed separately as doxygen fix.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode300
src/core/model/vector.h:300: Ptr<const AttributeChecker> MakeVectorChecker (void);
whitespace
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h
File src/core/model/wall-clock-synchronizer.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h#newcode209
src/core/model/wall-clock-synchronizer.h:209: virtual int64_t DoGetDrift (uint64_t ns);
similarly, this is a Doxygen update (can be separated)
Message from womenrgr8id@gmail.com
2018-02-26T13:03:20+00:00ammo6818-vandals.uidaho.eduurn:md5:d348f7bfa22797d4ac3fadc0f1fd3717
Added explanations to the review comments.
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc
File src/core/examples/hash-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc#newcode362
src/core/examples/hash-example.cc:362: long double k64 = static_cast<long double> (0xFFFFFFFFFFFFFFFFULL);
On 2018/02/25 18:14:57, Tom Henderson wrote:
> I am not sure about these; isn't long double an 8-byte quantity? why is a cast
> needed on k64 and not k32?
>
> Shouldn't it be something like std::numeric_limits<long double>::max()?
ULL is unsigned long long which different precision than long double hence the required downcast.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h
File src/core/model/assert.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h#newcode75
src/core/model/assert.h:75: } \
On 2018/02/25 18:14:57, Tom Henderson wrote:
> is this just a whitespace change?
alignment of continuation character
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h
File src/core/model/vector.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode252
src/core/model/vector.h:252: std::istream &operator >> (std::istream &is, Vector2D &vector);
On 2018/02/25 18:14:57, Tom Henderson wrote:
> this perhaps can be pulled out of this patch and committed separately as doxygen
> fix.
this was provided by Peter in a previous comment.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h
File src/core/model/wall-clock-synchronizer.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h#newcode209
src/core/model/wall-clock-synchronizer.h:209: virtual int64_t DoGetDrift (uint64_t ns);
On 2018/02/25 18:14:57, Tom Henderson wrote:
> similarly, this is a Doxygen update (can be separated)
this was provided by Peter in a previous comment
Message from pdbj@mac.com
2018-02-26T18:27:02+00:00Peter Barnesurn:md5:2162a67a543ad3d02bc199a0461bd8cc
I thought Robert revised patch 10, but it appears to be the same as last week, still containing doxy and whitespace changes I've been trying to remove. My most up to date suggestion is in the bug report, patch 3039, to apply on top of patch 8 here. As far as I can tell this still hasn't appeared here for review.
I won't be able to do anything more on this until Friday at the earliest. I don't think this should hold up ns-3.28 release.
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc
File src/core/examples/hash-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc#newcode362
src/core/examples/hash-example.cc:362: long double k64 = static_cast<long double> (0xFFFFFFFFFFFFFFFFULL);
On 2018/02/25 18:14:57, Tom Henderson wrote:
> I am not sure about these; isn't long double an 8-byte quantity? why is a cast
> needed on k64 and not k32?
>
> Shouldn't it be something like std::numeric_limits<long double>::max()?
I too am puzzled why casting is needed here, and not previous line.
This is the one patch I'm most unclear about. I need some time to stare at the code and remember why I did this value originally. I won't have time to do that until Friday.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h
File src/core/model/assert.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/assert.h#newcode75
src/core/model/assert.h:75: } \
On 2018/02/25 18:14:57, Tom Henderson wrote:
> is this just a whitespace change?
Yes both diffs are just whitespace to align '\'. Could be discarded from final patch.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h
File src/core/model/vector.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode252
src/core/model/vector.h:252: std::istream &operator >> (std::istream &is, Vector2D &vector);
On 2018/02/26 13:03:20, ammo6818-vandals.uidaho.edu wrote:
> On 2018/02/25 18:14:57, Tom Henderson wrote:
> > this perhaps can be pulled out of this patch and committed separately as
> doxygen
> > fix.
>
> this was provided by Peter in a previous comment.
Actually I've been trying to remove doxy from the VS patch. This is a holdover from Robert's very first patch.
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h
File src/core/model/wall-clock-synchronizer.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h#newcode209
src/core/model/wall-clock-synchronizer.h:209: virtual int64_t DoGetDrift (uint64_t ns);
On 2018/02/26 13:03:20, ammo6818-vandals.uidaho.edu wrote:
> On 2018/02/25 18:14:57, Tom Henderson wrote:
> > similarly, this is a Doxygen update (can be separated)
>
> this was provided by Peter in a previous comment
Ditto: part of Robert's original patch which I've been trying to remove.
Message from unknown
2018-02-27T13:10:42+00:00ammo6818-vandals.uidaho.eduurn:md5:fa2753e367eb33984ae3b2f39f4101ff
Message from womenrgr8id@gmail.com
2018-02-27T13:11:05+00:00ammo6818-vandals.uidaho.eduurn:md5:2d68dffa09b257bc2298a466b1a0d173
Updates posted for latest comments
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/command-line-example.cc
File src/core/examples/command-line-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/command-line-example.cc#newcode86
src/core/examples/command-line-example.cc:86: CommandLine cmd;
On 2018/02/25 18:14:56, Tom Henderson wrote:
> can be stripped from final patch (whitespace change)
change reverted
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc
File src/core/examples/hash-example.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/hash-example.cc#newcode362
src/core/examples/hash-example.cc:362: long double k64 = static_cast<long double> (0xFFFFFFFFFFFFFFFFULL);
On 2018/02/26 18:27:03, Peter Barnes wrote:
> On 2018/02/25 18:14:57, Tom Henderson wrote:
> > I am not sure about these; isn't long double an 8-byte quantity? why is a
> cast
> > needed on k64 and not k32?
> >
> > Shouldn't it be something like std::numeric_limits<long double>::max()?
>
> I too am puzzled why casting is needed here, and not previous line.
> This is the one patch I'm most unclear about. I need some time to stare at the
> code and remember why I did this value originally. I won't have time to do that
> until Friday.
The previous line does not require casting because it is a long to a long double which does not cause truncation of the numeric value.
The second line does because it is a long long to a long double which can cause a truncation of the numeric value.
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/main-test-sync.cc
File src/core/examples/main-test-sync.cc (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/examples/main-test-sync.cc#newcode45
src/core/examples/main-test-sync.cc:45: NS_LOG_COMPONENT_DEFINE ("TestSync");
On 2018/02/25 18:14:57, Tom Henderson wrote:
> can be stripped from final patch (whitespace change)
change reverted
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object-base.h
File src/core/model/object-base.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object-base.h#newcode82
src/core/model/object-base.h:82: namespace ns3 {
On 2018/02/25 18:14:57, Tom Henderson wrote:
> whitespace change
change reverted
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object.h
File src/core/model/object.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/object.h#newcode668
src/core/model/object.h:668: #endif /* OBJECT_H */
On 2018/02/25 18:14:57, Tom Henderson wrote:
> whitespace change
change reverted
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h
File src/core/model/vector.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode252
src/core/model/vector.h:252: std::istream &operator >> (std::istream &is, Vector2D &vector);
On 2018/02/26 18:27:03, Peter Barnes wrote:
> On 2018/02/26 13:03:20, http://ammo6818-vandals.uidaho.edu wrote:
> > On 2018/02/25 18:14:57, Tom Henderson wrote:
> > > this perhaps can be pulled out of this patch and committed separately as
> > doxygen
> > > fix.
> >
> > this was provided by Peter in a previous comment.
>
> Actually I've been trying to remove doxy from the VS patch. This is a holdover
> from Robert's very first patch.
doxygen updates removed and submitted as a separate patch
https://codereview.appspot.com/336970043/diff/260001/src/core/model/vector.h#newcode300
src/core/model/vector.h:300: Ptr<const AttributeChecker> MakeVectorChecker (void);
On 2018/02/25 18:14:57, Tom Henderson wrote:
> whitespace
change reverted
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h
File src/core/model/wall-clock-synchronizer.h (right):
https://codereview.appspot.com/336970043/diff/260001/src/core/model/wall-clock-synchronizer.h#newcode209
src/core/model/wall-clock-synchronizer.h:209: virtual int64_t DoGetDrift (uint64_t ns);
On 2018/02/26 18:27:03, Peter Barnes wrote:
> On 2018/02/26 13:03:20, http://ammo6818-vandals.uidaho.edu wrote:
> > On 2018/02/25 18:14:57, Tom Henderson wrote:
> > > similarly, this is a Doxygen update (can be separated)
> >
> > this was provided by Peter in a previous comment
>
> Ditto: part of Robert's original patch which I've been trying to remove.
doxygen updates removed and submitted as separate patch
Message from womenrgr8id@gmail.com
2018-03-02T23:22:44+00:00ammo6818-vandals.uidaho.eduurn:md5:95529e023e35aa0fb3efbcb10a769dde
One of the things that I am struggling with on this issue is who is on first and whats on second. I have lost track of what I need to be doing to bring this to completion.
A suggestion that I have is that you merge the files that are acceptable so that what is left is only the files that still have comments that need to be addressed which should make the patch set smaller and more manageable.
Message from unknown
2018-03-22T14:45:16+00:00ammo6818-vandals.uidaho.eduurn:md5:0ea9ac4d6d517dd2bb6653f6078a54d1
Message from womenrgr8id@gmail.com
2018-03-22T14:47:01+00:00ammo6818-vandals.uidaho.eduurn:md5:f7b2a53f472e6aacb18e44ba2c471275
I have reverified the patch includes off the changes provided in the two attachments and uploaded updates where I missed corrections from the attachments.
Message from pdbj@mac.com
2018-05-11T21:58:13+00:00Peter Barnesurn:md5:177b4ce1d23e32f48921357c5cc5d515
https://codereview.appspot.com/336970043/diff/300001/src/core/model/attribute-accessor-helper.h
File src/core/model/attribute-accessor-helper.h (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/attribute-accessor-helper.h#newcode392
src/core/model/attribute-accessor-helper.h:392: NS_UNUSED(v);
Previous two lines need space before paren.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc
File src/core/model/hash-murmur3.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode525
src/core/model/hash-murmur3.cc:525: m_hash32, (void *)& m_hash32);
Change size formal argument to size_t
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode526
src/core/model/hash-murmur3.cc:526: m_size32 += static_cast<uint32_t> (size);
Change m_size32 type to size_t
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode556
src/core/model/hash-murmur3.cc:556: MurmurHash3_x86_128_fin (static_cast<int> (m_size64),
Change size formal argument to size_t
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc
File src/core/model/heap-scheduler.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode156
src/core/model/heap-scheduler.cc:156: uint32_t index = static_cast<uint32_t> (Last ());
This has already been fixed, differently! in ns-3-dev.
std::size_t index = Last ();
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode220
src/core/model/heap-scheduler.cc:220: Exch (Root (), static_cast<uint32_t> (Last ()));
This has already been fixed, differently! in ns-3-dev.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode237
src/core/model/heap-scheduler.cc:237: Exch (i, static_cast<uint32_t> (Last ()));
This has already been fixed, differently! in ns-3-dev.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.cc
File src/core/model/log.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.cc#newcode714
src/core/model/log.cc:714:
This is already in upstream, and unrelated to this issue.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.h
File src/core/model/log.h (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.h#newcode531
src/core/model/log.h:531:
This is already in upstream, and unrelated to this issue.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/object-ptr-container.h
File src/core/model/object-ptr-container.h (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/object-ptr-container.h#newcode262
src/core/model/object-ptr-container.h:262: return (obj->*m_get)(static_cast<uint32_t> (i));
Unnecessary in ns-3-dev. If it is necessary, the fix is outside core, to update Get(), GetN() to size_t
https://codereview.appspot.com/336970043/diff/300001/src/core/model/realtime-simulator-impl.cc
File src/core/model/realtime-simulator-impl.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/realtime-simulator-impl.cc#newcode376
src/core/model/realtime-simulator-impl.cc:376: if (tsJitter > static_cast<uint64_t> (m_hardLimit.GetTimeStep ()))
Time::GetTimeStep returns int64_t. Can we change the declaration of tsJitter instead of casting? By pushing the cast into Synchronizer::GetCurrentRealtime
https://codereview.appspot.com/336970043/diff/300001/src/core/model/synchronizer.cc
File src/core/model/synchronizer.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/synchronizer.cc#newcode93
src/core/model/synchronizer.cc:93: return -static_cast<int64_t> (NanosecondToTimeStep (-tDrift));
Need to eliminate this cast as well, by chasing down the uint64_t declaration.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/type-id.cc
File src/core/model/type-id.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/type-id.cc#newcode451
src/core/model/type-id.cc:451: return static_cast<uint16_t> (uid);
Unnecessary cast; uid is already uint16_t
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc
File src/lte/test/lte-test-cell-selection.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode189
src/lte/test/lte-test-cell-selection.cc:189: uint16_t nUe = static_cast<uint16_t> (m_ueSetupList.size ());
Can nUe be size_t, so no cast would be necessary?
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode387
src/lte/test/lte-test-cell-selection.cc:387: NS_TEST_ASSERT_MSG_EQ (m_lastState.at (static_cast<unsigned int>(ueDev->GetImsi () - 1)),
Cast should be to size_t. What's the guarantee the -1 doesn't go negative?
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode401
src/lte/test/lte-test-cell-selection.cc:401: m_lastState.at (static_cast<unsigned int>(imsi - 1)) = newState;
Cast should be to size_t. What's the guarantee the -1 doesn't go negative?
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc
File src/lte/test/lte-test-phy-error-model.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc#newcode273
src/lte/test/lte-test-phy-error-model.cc:273: double expectedDlTxPackets = static_cast<double> (statsDuration.GetMilliSeconds ());
Use Time::ToDouble (Time::MS) instead of casting
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc#newcode429
src/lte/test/lte-test-phy-error-model.cc:429: double expectedDlTxPackets = static_cast<double> (statsDuration.GetMilliSeconds ());
Use Time::ToDouble (Time::MS) instead of casting
Message from womenrgr8id@gmail.com
2018-05-16T15:06:29+00:00ammo6818-vandals.uidaho.eduurn:md5:ee59f2bcd4c730d5599db8da8bee14d8
Updated patch submitted as https://codereview.appspot.com/342120043/ as requested.
I have responded to some of the questions in this patch about why a certain implementation could not be used.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/attribute-accessor-helper.h
File src/core/model/attribute-accessor-helper.h (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/attribute-accessor-helper.h#newcode392
src/core/model/attribute-accessor-helper.h:392: NS_UNUSED(v);
On 2018/05/11 21:58:11, Peter Barnes wrote:
> Previous two lines need space before paren.
Fixed
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc
File src/core/model/hash-murmur3.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode525
src/core/model/hash-murmur3.cc:525: m_hash32, (void *)& m_hash32);
On 2018/05/11 21:58:11, Peter Barnes wrote:
> Change size formal argument to size_t
Done
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode526
src/core/model/hash-murmur3.cc:526: m_size32 += static_cast<uint32_t> (size);
On 2018/05/11 21:58:11, Peter Barnes wrote:
> Change m_size32 type to size_t
Done
https://codereview.appspot.com/336970043/diff/300001/src/core/model/hash-murmur3.cc#newcode556
src/core/model/hash-murmur3.cc:556: MurmurHash3_x86_128_fin (static_cast<int> (m_size64),
On 2018/05/11 21:58:11, Peter Barnes wrote:
> Change size formal argument to size_t
Done
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc
File src/core/model/heap-scheduler.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode156
src/core/model/heap-scheduler.cc:156: uint32_t index = static_cast<uint32_t> (Last ());
On 2018/05/11 21:58:11, Peter Barnes wrote:
> This has already been fixed, differently! in ns-3-dev.
> std::size_t index = Last ();
removed
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode220
src/core/model/heap-scheduler.cc:220: Exch (Root (), static_cast<uint32_t> (Last ()));
On 2018/05/11 21:58:12, Peter Barnes wrote:
> This has already been fixed, differently! in ns-3-dev.
removed
https://codereview.appspot.com/336970043/diff/300001/src/core/model/heap-scheduler.cc#newcode237
src/core/model/heap-scheduler.cc:237: Exch (i, static_cast<uint32_t> (Last ()));
On 2018/05/11 21:58:12, Peter Barnes wrote:
> This has already been fixed, differently! in ns-3-dev.
removed
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.cc
File src/core/model/log.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/log.cc#newcode714
src/core/model/log.cc:714:
On 2018/05/11 21:58:12, Peter Barnes wrote:
> This is already in upstream, and unrelated to this issue.
One of the previous review comments was support for uint8_t needed to be added so this change in response to the other changes for this phase.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/object-ptr-container.h
File src/core/model/object-ptr-container.h (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/object-ptr-container.h#newcode262
src/core/model/object-ptr-container.h:262: return (obj->*m_get)(static_cast<uint32_t> (i));
On 2018/05/11 21:58:12, Peter Barnes wrote:
> Unnecessary in ns-3-dev. If it is necessary, the fix is outside core, to update
> Get(), GetN() to size_t
Removed.
https://codereview.appspot.com/336970043/diff/300001/src/core/model/realtime-simulator-impl.cc
File src/core/model/realtime-simulator-impl.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/realtime-simulator-impl.cc#newcode376
src/core/model/realtime-simulator-impl.cc:376: if (tsJitter > static_cast<uint64_t> (m_hardLimit.GetTimeStep ()))
On 2018/05/11 21:58:12, Peter Barnes wrote:
> Time::GetTimeStep returns int64_t. Can we change the declaration of tsJitter
> instead of casting? By pushing the cast into Synchronizer::GetCurrentRealtime
Making the suggested change causes a number of other casts to be created (the total number of casts required is larger).
https://codereview.appspot.com/336970043/diff/300001/src/core/model/type-id.cc
File src/core/model/type-id.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/core/model/type-id.cc#newcode451
src/core/model/type-id.cc:451: return static_cast<uint16_t> (uid);
On 2018/05/11 21:58:12, Peter Barnes wrote:
> Unnecessary cast; uid is already uint16_t
Removed
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc
File src/lte/test/lte-test-cell-selection.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode189
src/lte/test/lte-test-cell-selection.cc:189: uint16_t nUe = static_cast<uint16_t> (m_ueSetupList.size ());
On 2018/05/11 21:58:12, Peter Barnes wrote:
> Can nUe be size_t, so no cast would be necessary?
No. ueNodes.Create does not take a size_t
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode387
src/lte/test/lte-test-cell-selection.cc:387: NS_TEST_ASSERT_MSG_EQ (m_lastState.at (static_cast<unsigned int>(ueDev->GetImsi () - 1)),
On 2018/05/11 21:58:13, Peter Barnes wrote:
> Cast should be to size_t. What's the guarantee the -1 doesn't go negative?
changed to a size_t
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-cell-selection.cc#newcode401
src/lte/test/lte-test-cell-selection.cc:401: m_lastState.at (static_cast<unsigned int>(imsi - 1)) = newState;
On 2018/05/11 21:58:13, Peter Barnes wrote:
> Cast should be to size_t. What's the guarantee the -1 doesn't go negative?
changed to a size_t
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc
File src/lte/test/lte-test-phy-error-model.cc (right):
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc#newcode273
src/lte/test/lte-test-phy-error-model.cc:273: double expectedDlTxPackets = static_cast<double> (statsDuration.GetMilliSeconds ());
On 2018/05/11 21:58:13, Peter Barnes wrote:
> Use Time::ToDouble (Time::MS) instead of casting
Changed as noted.
https://codereview.appspot.com/336970043/diff/300001/src/lte/test/lte-test-phy-error-model.cc#newcode429
src/lte/test/lte-test-phy-error-model.cc:429: double expectedDlTxPackets = static_cast<double> (statsDuration.GetMilliSeconds ());
On 2018/05/11 21:58:13, Peter Barnes wrote:
> Use Time::ToDouble (Time::MS) instead of casting
Changed as noted.