New patch for core module changes

New patch for core module changes

[ammo6818-vandals.uidaho.edu, 2018-05-23 02:33:43]

Patch is ready for review.

[Peter Barnes, 2018-05-23 19:45:39]

1.  I have upstream patches addressing these issues, so these can be removed
from this code review:

// Add CommandLine to examples
src/core/examples/main-callback.cc
src/core/examples/main-ptr.cc
src/core/examples/main-random-variable-stream.cc
src/core/examples/main-random-variable.cc

// Correct class specification for fallback Demangle
src/core/model/callback.cc

// Use std::size_t for buffer size/len arguments
src/core/model/command-line.cc
src/core/model/hash-fnv.cc
src/core/model/hash-function.cc
src/core/model/hash-function.h
src/core/model/hash-murmur3.cc
src/core/model/hash-murmur3.h
src/core/model/hash.h
src/core/model/system-path.cc
src/core/test/hash-test-suite.cc

// Remove extraneous cast
src/core/model/type-id.cc


2.  Suggest we refactor the treatment of ATTRIBUTE_x
a.  In files which use the macros which declare/implement type-specific
attribute classes, we #include "ns3/attribute-helper.h" *after* the three-line
NS_EXPORT stanza. 

b.  Edit attribute-helper.h to include the symbol NS_EXPORT in the macros,
rather than passing the value through a macro argument.

This will enable the macros to insert the correct export attribute, based on the
enclosing file, and eliminate the additional clutter we have now.


3. I'd like to discuss refactoring the system-dependent functions in general,
but I'll raise that in a different thread.

4.  We need to discuss the deprecation process for old header names.  
In this case, we are effectively renaming unix-fd-reader.h to fd-reader.h.  I'll
make a suggestion in the email for #3.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h
File src/core/model/assert.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h#n...
src/core/model/assert.h:54: #include <string>
Is this needed?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h#n...
src/core/model/assert.h:74: # condition << "\", ";                              
 \
ws

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h#n...
src/core/model/assert.h:97: # condition << "\", ";                      \
ws

https://codereview.appspot.com/342120043/diff/20001/src/core/model/attribute-...
File src/core/model/attribute-accessor-helper.h (left):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/attribute-...
src/core/model/attribute-accessor-helper.h:392: NS_UNUSED(v);
These lines aren't in upstream ns-3-dev.  Is this code review based on the
correct commit?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/attribute-...
File src/core/model/attribute-helper.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/attribute-...
src/core/model/attribute-helper.h:47: *     -
ATTRIBUTE_HELPER_HEADER_LIB(type,lib) for classes that can be used by other
modules
The changes in this file should be minimal, just add NS3_EXPORT after "class".
See next file for additional comment.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/boolean.h
File src/core/model/boolean.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/boolean.h#...
src/core/model/boolean.h:88: ATTRIBUTE_ACCESSOR_DEFINE_LIB (Boolean,
NS3_EXPORT);
Ok, this highlights a subtlety in header ordering:  some core headers should go
*after* the ns3-export.h header.  Need to document which ones, and ensure those
*only* have module-dependent macros, nothing else.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/cairo-wide...
File src/core/model/cairo-wideint-private.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/cairo-wide...
src/core/model/cairo-wideint-private.h:108: NS3_EXPORT extern const char *
cairo_impl64;
Why does the declaration *and* the implementation have to be exported?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/config.h
File src/core/model/config.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/config.h#n...
src/core/model/config.h:24: #include "object.h"
This doesn't appear to be necessary with gcc: Ptr<Object> compiles fine with
just the forward declaration below.  If this is strictly necessary, then we
should remove the forward declaration.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64-c...
File src/core/model/int64x64-cairo.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64-c...
src/core/model/int64x64-cairo.h:256: friend NS3_EXPORT bool         operator ==
(const int64x64_t & lhs, const int64x64_t & rhs);
Why do these friend declarations have to be explicitly exported?  The
equivalents in nstime.h aren't.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64.h
File src/core/model/int64x64.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64.h...
src/core/model/int64x64.h:30: #include "ns3/int64x64-128.h"
Why do these includes have to be qualified by "ns3/"?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/names.cc
File src/core/model/names.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/names.cc#n...
src/core/model/names.cc:682: #ifndef _WIN32
Why is this block conditional?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-export.h
File src/core/model/ns3-export.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-export...
src/core/model/ns3-export.h:31: #else
Please label this else, and corresponding endif:
    #else  /* not __WIN32 */
    ...
    #endif  /* _WIN32 */

asdf

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-module.h
File src/core/model/ns3-module.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-module...
src/core/model/ns3-module.h:20: /* Make sure NS3_MODULE is undefined, it will be
defined by the file including this file. */
Is this file auto-generated? (I hope so...)  If so, it should state that in a
comment: 
    autogenerated by... Do not modify.  Each configured module is assigned a
unique name..

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-base.h
File src/core/model/object-base.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-bas...
src/core/model/object-base.h:71: #ifdef _WIN32
I don't understand this section.  Another header section which should follow the
3-line ns-export.h header?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-fac...
File src/core/model/object-factory.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-fac...
src/core/model/object-factory.h:123: friend NS3_EXPORT std::ostream & operator
<< (std::ostream &os, const ObjectFactory &factory);
Do the friend declaration and the real declaration both need to be marked for
export?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
File src/core/model/system-path.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.cc:363: #else
This block should appear before line 359, so they can share error reporting.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-path.h
File src/core/model/system-path.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.h:52: class NS3_EXPORT SystemPath
Why convert this namespace to a class?  To avoid explicit export of every
function?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.h:142: /**
Looks like these are being promoted from test-runner.cc?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-thr...
File src/core/model/system-thread.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-thr...
src/core/model/system-thread.cc:98: #ifndef _WIN32
Why is this conditional?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc
File src/core/model/test.cc (left):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc#ol...
src/core/model/test.cc:159: * Get the path to the root of the source tree.
Why are these moved to system-path.h?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc
File src/core/model/test.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc#ne...
src/core/model/test.cc:457: // The default value of NS_TEST_SOURCEDIR (__FILE__)
contains an absolute path instead of a relative path
Don't understand this section.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.h
File src/core/model/test.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.h#new...
src/core/model/test.h:1287: friend class NS3_EXPORT TestRunnerImpl;
Why does the impl have to be exported?  It's only forward declared in this file,
and used as a pointer.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/traced-val...
File src/core/model/traced-value.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/traced-val...
src/core/model/traced-value.h:158: : m_v (other.Get ())
Should this be instead
    : m_v ( (T)other.Get () )
Would that avoid the need to suppress warnings?  Or are we down casting from
wide to narrow?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/valgrind.h
File src/core/model/valgrind.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/valgrind.h...
src/core/model/valgrind.h:162: /* XXX: Unfortunately x64 Visual C++ does not
suport inline asms,
This should be defined by the build system, when necessary, not by modifying an
upstream file.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/valgrind.h...
src/core/model/valgrind.h:5278: (void)format;  // make compiler happy
Don't modify upstream file.  Need to find another way to suppress warnings in
this file.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/valgrind.h...
src/core/model/valgrind.h:5317: (void)format;  // make compiler happy
Don't modify upstream file.  Need to find another way to suppress warnings in
this file.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/vector.h
File src/core/model/vector.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/vector.h#n...
src/core/model/vector.h:79: friend NS3_EXPORT double CalculateDistance (const
Vector3D &a, const Vector3D &b);
Do both the friend declaration and formal declaration below need to be exported?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/wall-clock...
File src/core/model/wall-clock-synchronizer.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:336: #ifndef _WIN32
Why is this conditional?

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
File src/core/model/win32-fd-reader.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
src/core/model/win32-fd-reader.cc:157: #pragma warning(disable : 4389)
Document which warning message is being suppressed.

https://codereview.appspot.com/342120043/diff/20001/src/core/test/int64x64-te...
File src/core/test/int64x64-test-suite.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/test/int64x64-te...
src/core/test/int64x64-test-suite.cc:1000: #ifndef _MSC_VER
Refactor with following comment.

https://codereview.appspot.com/342120043/diff/20001/src/fd-net-device/model/f...
File src/fd-net-device/model/fd-net-device.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/fd-net-device/model/f...
src/fd-net-device/model/fd-net-device.h:36: #include "ns3/fd-reader.h"
Need to discuss deprecation process for old header name

[Peter Barnes, 2018-05-23 20:53:08]

On 2018/05/23 19:45:39, Peter Barnes wrote:
> 1.  I have upstream patches addressing these issues, so these can be removed
> from this code review:

Additional upstream updates:

// Consistent log component name
src/core/model/unix-system-wall-clock-ms.cc
src/core/model/win32-system-wall-clock-ms.cc

// Remove obsolete example
src/core/examples/main-random-variable.cc

And to clarify what wee need from system-dependent headers:
src/core/model/unix-system-condition.cc

[Peter Barnes, 2018-05-24 18:04:37]

Additional comments on fd-reader.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/fd-reader.h
File src/core/model/fd-reader.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/fd-reader....
src/core/model/fd-reader.h:22: #define WIN32_FD_READER_H
Wrong include guard symbol

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
File src/core/model/win32-fd-reader.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
src/core/model/win32-fd-reader.cc:69: // TODO: Windows implementation
Todo.  I think we need to see this file complete before deciding if and how to
refactor fd-reader.

[ammo6818-vandals.uidaho.edu, 2018-06-01 05:16:24]

Updated patch posted to address review comments.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h
File src/core/model/assert.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/assert.h#n...
src/core/model/assert.h:54: #include <string>
On 2018/05/23 19:45:40, Peter Barnes wrote:
> Is this needed?

Affirmative, some files in Network wont compile with out it.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/cairo-wide...
File src/core/model/cairo-wideint-private.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/cairo-wide...
src/core/model/cairo-wideint-private.h:108: NS3_EXPORT extern const char *
cairo_impl64;
On 2018/05/23 19:45:40, Peter Barnes wrote:
> Why does the declaration *and* the implementation have to be exported?

Removed from the .c file

https://codereview.appspot.com/342120043/diff/20001/src/core/model/config.h
File src/core/model/config.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/config.h#n...
src/core/model/config.h:24: #include "object.h"
On 2018/05/23 19:45:40, Peter Barnes wrote:
> This doesn't appear to be necessary with gcc: Ptr<Object> compiles fine with
> just the forward declaration below.  If this is strictly necessary, then we
> should remove the forward declaration.

Because DLLs are self contained execution units, they must be complete so
forward declarations of Ptr<> items are not sufficient.  An actual declaration
of the class is required (e.g. the declaration header file) so the Ref() and
UnRef() functions are known.

The forward class declaration was removed.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/fd-reader.h
File src/core/model/fd-reader.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/fd-reader....
src/core/model/fd-reader.h:22: #define WIN32_FD_READER_H
On 2018/05/24 18:04:36, Peter Barnes wrote:
> Wrong include guard symbol

Updated

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64-c...
File src/core/model/int64x64-cairo.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64-c...
src/core/model/int64x64-cairo.h:256: friend NS3_EXPORT bool         operator ==
(const int64x64_t & lhs, const int64x64_t & rhs);
On 2018/05/23 19:45:40, Peter Barnes wrote:
> Why do these friend declarations have to be explicitly exported?  The
> equivalents in nstime.h aren't.

Because the implementation of the functions are outside of the class so they
also have to be declared as NS3_EXPORT to match the implementation

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64.h
File src/core/model/int64x64.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/int64x64.h...
src/core/model/int64x64.h:30: #include "ns3/int64x64-128.h"
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why do these includes have to be qualified by "ns3/"?

Because this header file is included by modules outside of core, Intellisense
cant find the references for the items defined in these files without the folder
path.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/names.cc
File src/core/model/names.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/names.cc#n...
src/core/model/names.cc:682: #ifndef _WIN32
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why is this block conditional?

Windows throws an unreachable code error.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-export.h
File src/core/model/ns3-export.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-export...
src/core/model/ns3-export.h:31: #else
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Please label this else, and corresponding endif:
>     #else  /* not __WIN32 */
>     ...
>     #endif  /* _WIN32 */
> 
> asdf

Done.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-module.h
File src/core/model/ns3-module.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/ns3-module...
src/core/model/ns3-module.h:20: /* Make sure NS3_MODULE is undefined, it will be
defined by the file including this file. */
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Is this file auto-generated? (I hope so...)  If so, it should state that in a
> comment: 
>     autogenerated by... Do not modify.  Each configured module is assigned a
> unique name..

NS-3 core modules are not auto generated in Phase 1-4.  

In Phase 5,the file will be changed to be auto generated including contrib
modules.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-fac...
File src/core/model/object-factory.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/object-fac...
src/core/model/object-factory.h:123: friend NS3_EXPORT std::ostream & operator
<< (std::ostream &os, const ObjectFactory &factory);
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Do the friend declaration and the real declaration both need to be marked for
> export?

Yes, because the real declaration is outside of the class so it must be
NS3_EXPORT and the friend declaration must match the real declaration.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
File src/core/model/system-path.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.cc:363: #else
On 2018/05/23 19:45:41, Peter Barnes wrote:
> This block should appear before line 359, so they can share error reporting.

HAVE_MKDIR_H is always undefined for Visual Studio.

Error message corrected for Visual Studio

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-path.h
File src/core/model/system-path.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.h:52: class NS3_EXPORT SystemPath
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why convert this namespace to a class?  To avoid explicit export of every
> function?

So dllimport and dllexport could be used because these functions are used by
other modules after the Visual Studio port.

dllimport and dllexport cannot be used for namespaces, they require classes

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-pat...
src/core/model/system-path.h:142: /**
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Looks like these are being promoted from test-runner.cc?

They are needed by other modules and programs to access test files which are in
a different relative path to the executable file on the two platforms so these
functions are used to make a path from the root folder.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-thr...
File src/core/model/system-thread.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/system-thr...
src/core/model/system-thread.cc:98: #ifndef _WIN32
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why is this conditional?

Visual Studio implementation cannot determine an output operator.  Added code to
output address on Windows.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc
File src/core/model/test.cc (left):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc#ol...
src/core/model/test.cc:159: * Get the path to the root of the source tree.
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why are these moved to system-path.h?

Because they are used by code outside of core so they need to be exportable from
core.dll

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc
File src/core/model/test.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.cc#ne...
src/core/model/test.cc:457: // The default value of NS_TEST_SOURCEDIR (__FILE__)
contains an absolute path instead of a relative path
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Don't understand this section.

NS_TEST_SOURCEDIR was previously set by WAF.  Under VS, its set as a defined
symbol in the project settings using the predefined symbol __FILE__. __FILE__
creates an absolute file reference while the WAF setting is a relative path
reference.  This code creates a relative path reference to match the WAF
implementation.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.h
File src/core/model/test.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/test.h#new...
src/core/model/test.h:1287: friend class NS3_EXPORT TestRunnerImpl;
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Why does the impl have to be exported?  It's only forward declared in this
file,
> and used as a pointer.

Removed.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/traced-val...
File src/core/model/traced-value.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/traced-val...
src/core/model/traced-value.h:158: : m_v (other.Get ())
On 2018/05/23 19:45:41, Peter Barnes wrote:
> Should this be instead
>     : m_v ( (T)other.Get () )
> Would that avoid the need to suppress warnings?  Or are we down casting from
> wide to narrow?

Added (T) and removed the #pragma.  Good catch!!

https://codereview.appspot.com/342120043/diff/20001/src/core/model/vector.h
File src/core/model/vector.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/vector.h#n...
src/core/model/vector.h:79: friend NS3_EXPORT double CalculateDistance (const
Vector3D &a, const Vector3D &b);
On 2018/05/23 19:45:42, Peter Barnes wrote:
> Do both the friend declaration and formal declaration below need to be
exported?

Yes, because the function is defined outside of the class, it is required for
the implementation and the reference must match the implmentation.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/wall-clock...
File src/core/model/wall-clock-synchronizer.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:336: #ifndef _WIN32
On 2018/05/23 19:45:42, Peter Barnes wrote:
> Why is this conditional?

Because it generates a code cannot be reached warning under VS.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
File src/core/model/win32-fd-reader.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
src/core/model/win32-fd-reader.cc:69: // TODO: Windows implementation
On 2018/05/24 18:04:36, Peter Barnes wrote:
> Todo.  I think we need to see this file complete before deciding if and how to
> refactor fd-reader.

Windows implementation of this is a Phase 5 or Phase 6 effort and wont be done
for some time.

Initial VS changes are being implemented only for compilation success on VS.

https://codereview.appspot.com/342120043/diff/20001/src/core/model/win32-fd-r...
src/core/model/win32-fd-reader.cc:157: #pragma warning(disable : 4389)
On 2018/05/23 19:45:42, Peter Barnes wrote:
> Document which warning message is being suppressed.

Added.

https://codereview.appspot.com/342120043/diff/20001/src/core/test/int64x64-te...
File src/core/test/int64x64-test-suite.cc (right):

https://codereview.appspot.com/342120043/diff/20001/src/core/test/int64x64-te...
src/core/test/int64x64-test-suite.cc:1000: #ifndef _MSC_VER
On 2018/05/23 19:45:42, Peter Barnes wrote:
> Refactor with following comment.

Its unclear what change you are requesting.

https://codereview.appspot.com/342120043/diff/20001/src/fd-net-device/model/f...
File src/fd-net-device/model/fd-net-device.h (right):

https://codereview.appspot.com/342120043/diff/20001/src/fd-net-device/model/f...
src/fd-net-device/model/fd-net-device.h:36: #include "ns3/fd-reader.h"
On 2018/05/23 19:45:42, Peter Barnes wrote:
> Need to discuss deprecation process for old header name

Change old header to include comment about deprecation.