Visual Studio conditional code for core module

Visual Studio conditional code for core module

[Peter Barnes, 2018-03-30 05:11:18]

Where does _CORE get defined, to toggle between building core vs. referencing
core?

[ammo6818-vandals.uidaho.edu, 2018-03-30 23:59:04]

On 2018/03/30 05:11:18, Peter Barnes wrote:
> Where does _CORE get defined, to toggle between building core vs. referencing
> core?

It gets defined in the Visual Studio project settings for the core module DLL. 
Its defined when building the DLL and not defined when building any other module
or program.

[ammo6818-vandals.uidaho.edu, 2018-04-05 17:47:22]

Updated patch set uploaded to address review comments.

[ammo6818-vandals.uidaho.edu, 2018-04-06 19:10:11]

Changes incorporated for ATTRIBUTE_* macros to address review comments.  Latest
module changes alos incorporated.

[ammo6818-vandals.uidaho.edu, 2018-04-16 15:14:09]

Please let me know if you have any additional review comments I need to address.

[Peter Barnes, 2018-04-17 00:04:11]

Hello Robert,

Here are detailed comments on peripheral issues, and you may have seen some
related commits which enable cleaner ways forward.

At the big picture level, I have a couple of questions:

*  Apparently there are other ways to indicate export/import:
"For Microsoft Windows targets there are alternative methods for including the
symbol in the DLL’s export table such as using a .def file with an EXPORTS
section or, with GNU ld, using the --export-all linker flag. "[1]

Can you discuss, maybe on the wiki, the relative merits of your approach and
these approaches? Why are these not feasible for ns-3 on Windows?

*  I don’t see the build system code which sets the compilation module.  Will
this use the existing NS3_MODULE_x symbol?

*  IIUC, the hard part is this (which we do a lot, but is somewhat unusual in
general):

    foo/model/foo.h:
      class foo {…};

    bar/model/bar.h:
      #include “ns3/foo.h”
    
      class bar {…};

The trick is that when compiling module foo, the symbols in foo.h should be
labeled dllexport.  When compiling bar.cc, those same symbols in foo.h should be
labeled dllimport. Could you elaborate on this on the wiki, so everyone can
better understand the issues?

*  Why don't all headers include ns3dll.h?

*  Would this work as an alternative approach to your ns3dll.h?

src/core/model/ns3-export.h:
	//  No include guard.  This is meant to appear many times in a single
compilation unit

	/*Usage:  start each header with:

		  #define NS3_MODULE myModule
		  #include "ns3/ns-export.h"

	Label each class with

		  class NS3_EXPORT ...

	Have waf define NS3_COMPILATION_MODULE as the current module.
	(Or repurpose what it currently defines, NS3_MODULE_COMPILATION=1
	to hold the module name.)
	*/
	
	#ifdef NS3_EXPORT
	#  undef NS3_EXPORT
	#endif

	# From
https://stackoverflow.com/questions/2335888/how-to-compare-strings-in-c-condi...
	constexpr int
	NS3_MODULE_STRCMP (char const* lhs, char const* rhs)
	{
	  return (('\0' == lhs[0]) && ('\0' == rhs[0])) ? 0
		: (lhs[0] != rhs[0]) ? ((int)lhs[0] - (int)rhs[0])
		: NS3_MODULE_STRCMP ( lhs + 1, rhs + 1 );
	}

	#if 0 == NS3_MODULE_STRCMP (NS3_MODULE_COMPILATION, NS3_MODULE)
	#  define NS3_EXPORT __declspec(dllexport)
	#else
	#  define NS3_EXPORT __declspec(dllimport)
	#endif

Peter

[1]
https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html...

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-ca...
File src/core/examples/main-callback.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-ca...
src/core/examples/main-callback.cc:22: #include "ns3/command-line.h"
Also need to add this to each program, at the top of main():

CommandLine cmd;
cmd.Parse (argc, arg);

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-te...
File src/core/examples/main-test-sync.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-te...
src/core/examples/main-test-sync.cc:30: #ifndef _WIN32
I've modified this file to use std::this_thread::sleep_for
(std::chrono::seconds(1));  shouldn't need any Win-specific code.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:51: *     - ATTRIBUTE_HELPER_CPP(type,lib) for
self contained programs
Need to clarify what's a library, and what's a self-contained program.  Why
can't these be unified?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:213: #define
ATTRIBUTE_VALUE_DEFINE_WITH_NAME(type,name)                     \
Can't this be implemented as

#define ATTRIBUTE_VALUE_DEFINE_WITH_NAME(type,name)                     \
    ATTRIBUTE_VALUE_DEFINE_WITH_NAME_LIB(type,name,)

Then we wouldn't need to duplicate this macro body.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:319:
ATTRIBUTE_VALUE_IMPLEMENT_WITH_NAME(type,name)
All the implementation macros discard the lib argument.  If so, why add the
argument?  Just to parallel the _DEFINE_LIB macros?  Seems unnecessary.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/callback.cc
File src/core/model/callback.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/callback.c...
src/core/model/callback.cc:129: ns3::CallbackImplBase::Demangle (const
std::string& mangled)
Isn't this a bug in the current code?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
File src/core/model/command-line.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
src/core/model/command-line.h:33: #endif
This should be in the .cc file, not the header.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
src/core/model/command-line.h:373: *  Function to force static initialization of
Time.
"CommandLine"

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
src/core/model/command-line.h:538: static bool
NS_UNUSED_GLOBAL(g_CommandLineStaticInit) = CommandLine::StaticInit();
Can this be done in the CommandLine constructor?  Instead of statically?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl.cc
File src/core/model/fatal-impl.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl...
src/core/model/fatal-impl.cc:210: std::list<std::ostream*> *l = *pl;
Seems like only 4-5 lines in this function are system dependent, so let's just
put conditionals around them, so it's clear we're basically doing the same
thing.

The conditional lines are the preceding three...

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl...
src/core/model/fatal-impl.cc:221: signal(SIGSEGV, previousHandler);
...and this one (which is two in the 'nix version)

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
File src/core/model/log-macros-disabled.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-disabled.h:40: }                                      
\
Nope, can't delete the 'nix definition.  Also there's a new macro in this file I
just pushed.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
File src/core/model/log-macros-enabled.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-enabled.h:86: __func__ << "(): ";                     
                 \
Discussed on the wiki:  can't we just use __func__ on both systems?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-enabled.h:214: << __func__ << "()" << std::endl;      
    \
Ditto

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-enabled.h:269: << __func__ << "(";                    
    \
Ditto

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/names.cc#n...
src/core/model/names.cc:684: return 0;
What's troublesome with this line?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/ns3dll.h
File src/core/model/ns3dll.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/ns3dll.h#n...
src/core/model/ns3dll.h:22: #ifdef _ANT
This doesn't scale.  Adding a module requires updating a core file!?  What about
modules in contrib?

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-bas...
src/core/model/object-base.h:88: #define
NS_OBJECT_TEMPLATE_CLASS_DEFINE(type,param)                    \
Can't this be implemented as

#define NS_OBJECT_TEMPLATE_CLASS_DEFINE(type,param)            \
    NS_OBJECT_TEMPLATE_CLASS_DEFINE_LIB(type,param,)

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-ptr...
File src/core/model/object-ptr-container.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-ptr...
src/core/model/object-ptr-container.h:262: return
(obj->*m_get)(static_cast<uint32_t> (i));
I think this is a bug:  the three uses of std::size_t should be the template
parameter INDEX

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/system-pat...
src/core/model/system-path.cc:339: #if defined(HAVE_MKDIR_H)
1.  I've refactored this function to eliminate the extra create directory below.

2.  I've refactored this conditional section:

  bool makeDirErr;

#if ...
  makeDirErr = ...
#else
  makeDirErr = ...
#endif

  if (makeDirErr) ...

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/system-thr...
src/core/model/system-thread.cc:99: NS_LOG_FUNCTION (id);
What's wrong with this line?

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/test.cc#ne...
src/core/model/test.cc:471: #else
Not clear what all this does.  Could you please elaborate what this does, why
it's needed?

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/traced-val...
src/core/model/traced-value.h:158: : m_v (other.Get ())
Instead, shouldn't this be a cast from U to T, as below?

  : m_v ( (T) (other.Get ()) )

https://codereview.appspot.com/342870043/diff/60001/src/core/model/uinteger.h
File src/core/model/uinteger.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/uinteger.h...
src/core/model/uinteger.h:83: #include "ns3dll.h"
Doesnt' this have to be before the use of NS3CORELIB above?

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-fd-re...
File src/core/model/unix-fd-reader.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-fd-re...
src/core/model/unix-fd-reader.cc:24: #ifndef _WIN32
We shouldn't have conditional compilation in this file; it's the unix
implementation of FdREader.

We should create an generic header, fd-reader.h

We should select which to build (unix-fd-reader.cc or win32-rd-reader.cc) in the
wscript, not in conditional compilation.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-syste...
File src/core/model/unix-system-condition.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-syste...
src/core/model/unix-system-condition.cc:21: #ifndef _WIN32
See comment on unix-fd-reader.cc

And this should be in a separate header, suggest win32-time.h

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:25: #ifndef _FILETIME_
This should probably be in a separate file, suggest win32-time.{h,cc}

https://codereview.appspot.com/342870043/diff/60001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:408: #ifndef _WIN32
Why is this necessary?

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

https://codereview.appspot.com/342870043/diff/60001/src/core/test/int64x64-te...
src/core/test/int64x64-test-suite.cc:1004: // Darwin 12.5.0 (Mac 10.8.5) g++
4.2.1
This should be in a conditional

https://codereview.appspot.com/342870043/diff/60001/src/core/test/int64x64-te...
src/core/test/int64x64-test-suite.cc:1007: #ifdef _MSC_VER
This should be in there previous if clause

https://codereview.appspot.com/342870043/diff/60001/src/core/test/threaded-te...
File src/core/test/threaded-test-suite.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/test/threaded-te...
src/core/test/threaded-test-suite.cc:110:
std::this_thread::sleep_for(std::chrono::nanoseconds(500));
Won't this work on Win?  It's standard C++-11.

[ammo6818-vandals.uidaho.edu, 2018-05-02 05:06:32]

I have posted an update based upon your review comments.  The  update includes
resolution of your comments for the phase 2 changes to core (and the other
affected modules) and phase 3 changes to core based upon your alternate
proposal.

The one thing that wont work from your alternate proposal is auto generation of
the ordinal numbers for the modules.  There isnt the ability to do something
equivalent to ./waf configure as a prebuild step and if this file is auto
generated it will cause a re compile of all files any time a build is performed.

I dont think there is an issue of statically defined ordinals for the ns-3 core
modules.  If modules are dropped from ns-3 core the ordinals are not reused.

The only issue would be contrib module conflicts with other contrib modules. 
But that issue would not be unique to ordinals, the same problem would exist for
header and source file names.

I really dont have a better alternative for this issue.

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-ca...
File src/core/examples/main-callback.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-ca...
src/core/examples/main-callback.cc:22: #include "ns3/command-line.h"
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Also need to add this to each program, at the top of main():
> 
> CommandLine cmd;
> cmd.Parse (argc, arg);

Changed as noted.

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-te...
File src/core/examples/main-test-sync.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/examples/main-te...
src/core/examples/main-test-sync.cc:30: #ifndef _WIN32
On 2018/04/17 00:04:12, Peter Barnes wrote:
> I've modified this file to use std::this_thread::sleep_for
> (std::chrono::seconds(1));  shouldn't need any Win-specific code.

Ok

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:51: *     - ATTRIBUTE_HELPER_CPP(type,lib) for
self contained programs
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Need to clarify what's a library, and what's a self-contained program.  Why
> can't these be unified?

Changed to:

 * There are two kinds of helper macros:
 *   -# The simple macros.
 *     - ATTRIBUTE_HELPER_HEADER_LIB(type,lib) for classes that can be used by
other modules
 *     - ATTRIBUTE_HELPER_CPP_LIB(type,lib) for classes that can be used by
other modules
 *       or
 *     - ATTRIBUTE_HELPER_HEADER(type,lib) for classes that are only used in the
this executable unit
 *     - ATTRIBUTE_HELPER_CPP(type,lib) for classes that are only used in the
this executable unit
 *   -# The more complex macros.
 *

See attribute-test-suite.cc for an example of the latter.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:213: #define
ATTRIBUTE_VALUE_DEFINE_WITH_NAME(type,name)                     \
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Can't this be implemented as
> 
> #define ATTRIBUTE_VALUE_DEFINE_WITH_NAME(type,name)                     \
>     ATTRIBUTE_VALUE_DEFINE_WITH_NAME_LIB(type,name,)
> 
> Then we wouldn't need to duplicate this macro body.

No,there is a subtle difference on line 191.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/attribute-...
src/core/model/attribute-helper.h:319:
ATTRIBUTE_VALUE_IMPLEMENT_WITH_NAME(type,name)
On 2018/04/17 00:04:12, Peter Barnes wrote:
> All the implementation macros discard the lib argument.  If so, why add the
> argument?  Just to parallel the _DEFINE_LIB macros?  Seems unnecessary.

This was done to make it easier for the developers so they all had the same
calling format.  In my opinion, it would make them harder to use if there were
different parameter sequences and users had to know which macro had what format.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/callback.cc
File src/core/model/callback.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/callback.c...
src/core/model/callback.cc:129: ns3::CallbackImplBase::Demangle (const
std::string& mangled)
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Isn't this a bug in the current code?

Affirmative, for the Windows (old cygwin?) configuration only.  Since its a bug
in the Windows configuration only, I have addressed it in the Windows only
changes.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
File src/core/model/command-line.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/command-li...
src/core/model/command-line.h:373: *  Function to force static initialization of
Time.
On 2018/04/17 00:04:12, Peter Barnes wrote:
> "CommandLine"

Corrected reference to Time.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl.cc
File src/core/model/fatal-impl.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl...
src/core/model/fatal-impl.cc:210: std::list<std::ostream*> *l = *pl;
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Seems like only 4-5 lines in this function are system dependent, so let's just
> put conditionals around them, so it's clear we're basically doing the same
> thing.
> 
> The conditional lines are the preceding three...

Updated.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/fatal-impl...
src/core/model/fatal-impl.cc:221: signal(SIGSEGV, previousHandler);
On 2018/04/17 00:04:12, Peter Barnes wrote:
> ...and this one (which is two in the 'nix version)

Updated.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
File src/core/model/log-macros-disabled.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-disabled.h:40: }                                      
\
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Nope, can't delete the 'nix definition.  Also there's a new macro in this file
I
> just pushed.

Ok

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
File src/core/model/log-macros-enabled.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-enabled.h:86: __func__ << "(): ";                     
                 \
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Discussed on the wiki:  can't we just use __func__ on both systems?

Unknown if that works on all platforms and I don't have a way to verify.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/log-macros...
src/core/model/log-macros-enabled.h:214: << __func__ << "()" << std::endl;      
    \
On 2018/04/17 00:04:12, Peter Barnes wrote:
> Ditto

Ditto.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/names.cc#n...
src/core/model/names.cc:684: return 0;
On 2018/04/17 00:04:12, Peter Barnes wrote:
> What's troublesome with this line?

Leaving this uncommented causes a code cannot be reached error.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-bas...
src/core/model/object-base.h:88: #define
NS_OBJECT_TEMPLATE_CLASS_DEFINE(type,param)                    \
On 2018/04/17 00:04:13, Peter Barnes wrote:
> Can't this be implemented as
> 
> #define NS_OBJECT_TEMPLATE_CLASS_DEFINE(type,param)            \
>     NS_OBJECT_TEMPLATE_CLASS_DEFINE_LIB(type,param,)

No, lines 70 and 71 are different.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-ptr...
File src/core/model/object-ptr-container.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/object-ptr...
src/core/model/object-ptr-container.h:262: return
(obj->*m_get)(static_cast<uint32_t> (i));
On 2018/04/17 00:04:13, Peter Barnes wrote:
> I think this is a bug:  the three uses of std::size_t should be the template
> parameter INDEX

I had to do this to resolve a Visual Studio compiler warning in one of the
modules.  All examples and test function with this change.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/system-pat...
src/core/model/system-path.cc:339: #if defined(HAVE_MKDIR_H)
On 2018/04/17 00:04:13, Peter Barnes wrote:
> 1.  I've refactored this function to eliminate the extra create directory
below.
> 
> 2.  I've refactored this conditional section:
> 
>   bool makeDirErr;
> 
> #if ...
>   makeDirErr = ...
> #else
>   makeDirErr = ...
> #endif
> 
>   if (makeDirErr) ...

OK

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/system-thr...
src/core/model/system-thread.cc:99: NS_LOG_FUNCTION (id);
On 2018/04/17 00:04:13, Peter Barnes wrote:
> What's wrong with this line?

Error	C2679	binary '<<': no operator found which takes a right-hand operand of
type 'ns3::SystemThread::ThreadId' (or there is no acceptable conversion)

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/test.cc#ne...
src/core/model/test.cc:471: #else
On 2018/04/17 00:04:13, Peter Barnes wrote:
> Not clear what all this does.  Could you please elaborate what this does, why
> it's needed?  

Under Linux, NS_TEST_SOURCEDIR is defined by WAF.  Under Windows, the value of
NS_TEST_SOURCEDIR is defined by declaring it as NS_TEST_SOURCEDIR (__FILE__) in
the project settings file.

Using this approach, the value of NS_TEST_SOURCEDIR ends up being defined
slightly differently which the conditional code handles.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/traced-val...
src/core/model/traced-value.h:158: : m_v (other.Get ())
On 2018/04/17 00:04:13, Peter Barnes wrote:
> Instead, shouldn't this be a cast from U to T, as below?
> 
>   : m_v ( (T) (other.Get ()) )

That change just suppresses the warning.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/uinteger.h
File src/core/model/uinteger.h (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/uinteger.h...
src/core/model/uinteger.h:83: #include "ns3dll.h"
On 2018/04/17 00:04:13, Peter Barnes wrote:
> Doesnt' this have to be before the use of NS3CORELIB above?

Removed.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-fd-re...
File src/core/model/unix-fd-reader.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-fd-re...
src/core/model/unix-fd-reader.cc:24: #ifndef _WIN32
On 2018/04/17 00:04:13, Peter Barnes wrote:
> We shouldn't have conditional compilation in this file; it's the unix
> implementation of FdREader.
> 
> We should create an generic header, fd-reader.h
> 
> We should select which to build (unix-fd-reader.cc or win32-rd-reader.cc) in
the
> wscript, not in conditional compilation.

changed to Win32 specific source files with a common header file

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-syste...
File src/core/model/unix-system-condition.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/model/unix-syste...
src/core/model/unix-system-condition.cc:21: #ifndef _WIN32
On 2018/04/17 00:04:13, Peter Barnes wrote:
> See comment on unix-fd-reader.cc
> 
> And this should be in a separate header, suggest win32-time.h

Moved Windows declarations to a separate header file.

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

https://codereview.appspot.com/342870043/diff/60001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:25: #ifndef _FILETIME_
On 2018/04/17 00:04:13, Peter Barnes wrote:
> This should probably be in a separate file, suggest win32-time.{h,cc}

Move to separate files.

https://codereview.appspot.com/342870043/diff/60001/src/core/model/wall-clock...
src/core/model/wall-clock-synchronizer.cc:408: #ifndef _WIN32
On 2018/04/17 00:04:13, Peter Barnes wrote:
> Why is this necessary?

Because it generates a never reachable code error (because its code that is
never reachable).

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

https://codereview.appspot.com/342870043/diff/60001/src/core/test/int64x64-te...
src/core/test/int64x64-test-suite.cc:1007: #ifdef _MSC_VER
On 2018/04/17 00:04:13, Peter Barnes wrote:
> This should be in there previous if clause

Windows implementation is cairo_impl so its outside of the if clause.  epsilon
initialization move up to where other epsilon declaration is located.

https://codereview.appspot.com/342870043/diff/60001/src/core/test/threaded-te...
File src/core/test/threaded-test-suite.cc (right):

https://codereview.appspot.com/342870043/diff/60001/src/core/test/threaded-te...
src/core/test/threaded-test-suite.cc:110:
std::this_thread::sleep_for(std::chrono::nanoseconds(500));
On 2018/04/17 00:04:14, Peter Barnes wrote:
> Won't this work on Win?  It's standard C++-11.

incorporated your change.