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.