Bug 2457

Summary: Use variadic templates to support arbitrary number of arguments
Product: ns-3 Reporter: Alexander Krotov <krotov>
Component: coreAssignee: Peter Barnes <pdbarnes>
Status: PATCH PENDING ---    
Severity: enhancement CC: natale.patriciello, ns-bugs, stavallo, tommaso.pecorella
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
See Also: https://www.nsnam.org/bugzilla/show_bug.cgi?id=2203
Bug Depends on:    
Bug Blocks: 2251    
Attachments: Proposed patch
Proposed patch
Proposed patch
Proposed patch
Revised doxygen
Proposed patch

Description Alexander Krotov 2016-07-30 16:19:50 EDT
Created attachment 2505 [details]
Proposed patch

Since NS-3 switched to C++11 it makes sense to use variadic templates instead of repeated "T1, T2, T3, ...". I prepared a patch that makes use of variadic templates where it was obvious how to change the code. "grep T5" still shows that callback.h, make-event.h and timer-impl.h require changes.
Comment 1 Peter Barnes 2016-08-02 20:05:38 EDT
Very nice.  It's impressive how many lines are removed, almost 1700!, and only 100 inserted.

I've wanted to do this for a long time, but...

> Since NS-3 switched to C++11

We haven't switched to C++11; we now *support* C++11, meaning releases (and ns-3-dev) which are C++98 codes build cleanly with --std=c++11  This enables users to write their scenarios, in scratch for example, using C++11 constructs.

We can't apply this patch until the project decides to stop supporting C++98.

However, as a teaser we could consider a patch which conditionally supplies the C++11 version:

#if __cplusplus > 199711L
template <typename T, typename... Args>
Ptr<T> CreateObject (Args... args)
{
  return CompleteConstruct (new T (args...));
}
...

#else
// Legacy C++98 versions
...
#endif

I'll leave this at PATCH WANTED in case you want to go that route, otherwise I'll eventually mark this as RESOLVED LATER
Comment 2 Peter Barnes 2016-08-19 00:30:47 EDT
On Aug 18, 2016, at 6:08 PM, Tom Henderson <tomh@tomh.org> wrote:

Hi Peter,
I'm not sure what you mean by this patch not being ready.  We now build with --std=c++11 for all modules (there is some dependent code in there already).  We require g++-4.8 and above nowadays.

I tried Alexander's patch on ns-3-dev just now and it built and all tests passed.

I wouldn't push it just now, but after release?  Or am I missing something?

- Tom
Comment 3 Peter Barnes 2016-08-19 00:32:39 EDT
In reply to Tom Henderson:
I was about to write:
> Yes we build correctly with --std=c++11, but that isn’t the default.  This patch
> won’t build with the default of older gcc, effectively —std=c++98 or —std=c++03.
> 
> As to “we require g++-4.8”, since when?  That’s not available on a stock Mac with
> Developer Tools (at least that’s how I read the version message):
>> $ gcc --version
>> Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx->> include-dir=/usr/include/c++/4.2.1
>> Apple LLVM version 7.0.2 (clang-700.1.81)
>> Target: x86_64-apple-darwin14.5.0
>> Thread model: posix
> Notice the use g++-4.2.1 headers, definitely pre C++11

Whoops!  I just checked.  You made --std=c++11 the default in May when I wasn’t looking.  Ok, no issues with this patch.  I’ll update the tracker.

Marking this as PATCH PENDING until we get it pushed, after the 3.26 release.
Comment 4 Alexander Krotov 2016-10-20 12:31:53 EDT
Created attachment 2630 [details]
Proposed patch

Updated patch to remove even more "ladders".

3.26 is out, how about pushing this patch?
Comment 5 Peter Barnes 2016-10-20 16:24:35 EDT
I've started to look at this patch.  As code it's great.  I'm refining the 
documentation, which has some issues.  I should be able to have a revised patch 
in about a week.
Comment 6 Tommaso Pecorella 2016-10-20 16:38:14 EDT
On a MacOS the patch raises some errors, like:

../examples/energy/energy-model-example.cc:87:7: error: call to 'Schedule' is ambiguous
      Simulator::Schedule (pktInterval, &GenerateTraffic, socket, pktSize, n,
      ^~~~~~~~~~~~~~~~~~~
./ns3/simulator.h:523:20: note: candidate function [with MEM = void (*)(ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time), OBJ = ns3::Ptr<ns3::Socket>, Ts = <unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, MEM mem_ptr, OBJ obj, Ts... args)
                   ^
./ns3/simulator.h:529:20: note: candidate function [with Us = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>, Ts = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, void (*f)(Us...), Ts... args)
                   ^
../examples/energy/energy-model-example.cc:264:3: error: call to 'Schedule' is ambiguous
  Simulator::Schedule (Seconds (startTime), &GenerateTraffic, source, PpacketSize,
  ^~~~~~~~~~~~~~~~~~~
./ns3/simulator.h:523:20: note: candidate function [with MEM = void (*)(ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time), OBJ = ns3::Ptr<ns3::Socket>, Ts = <unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, MEM mem_ptr, OBJ obj, Ts... args)
                   ^
./ns3/simulator.h:529:20: note: candidate function [with Us = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>, Ts = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, void (*f)(Us...), Ts... args)
                   ^
2 errors generated.

../examples/energy/energy-model-with-harvesting-example.cc:116:7: error: call to 'Schedule' is ambiguous
      Simulator::Schedule (pktInterval, &GenerateTraffic, socket, pktSize, n,
      ^~~~~~~~~~~~~~~~~~~
./ns3/simulator.h:523:20: note: candidate function [with MEM = void (*)(ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time), OBJ = ns3::Ptr<ns3::Socket>, Ts = <unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, MEM mem_ptr, OBJ obj, Ts... args)
                   ^
./ns3/simulator.h:529:20: note: candidate function [with Us = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>, Ts = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, void (*f)(Us...), Ts... args)
                   ^
../examples/energy/energy-model-with-harvesting-example.cc:329:3: error: call to 'Schedule' is ambiguous
  Simulator::Schedule (Seconds (startTime), &GenerateTraffic, source, PpacketSize,
  ^~~~~~~~~~~~~~~~~~~
./ns3/simulator.h:523:20: note: candidate function [with MEM = void (*)(ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time), OBJ = ns3::Ptr<ns3::Socket>, Ts = <unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, MEM mem_ptr, OBJ obj, Ts... args)
                   ^
./ns3/simulator.h:529:20: note: candidate function [with Us = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>, Ts = <ns3::Ptr<ns3::Socket>, unsigned int, ns3::Ptr<ns3::Node>, unsigned int, ns3::Time>]
EventId Simulator::Schedule (Time const &delay, void (*f)(Us...), Ts... args)
                   ^
2 errors generated.


Compiler info:

22:38:06:~/Development/workspace/ns-3-dev pecos$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 8.0.0 (clang-800.0.38)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
Comment 7 Peter Barnes 2016-10-20 16:46:37 EDT
Hmm, I didnt' have errors using gcc-5.4.0
Comment 8 Alexander Krotov 2016-10-20 16:49:52 EDT
(In reply to Peter Barnes from comment #7)
> Hmm, I didnt' have errors using gcc-5.4.0

I actually got these errors on Linux after

export CXX=clang++
export CC=clang
./waf configure -d debug
./waf

gcc --version is probably unrelated here as on Mac default compiler is clang.
Comment 9 Peter Barnes 2016-10-20 16:51:36 EDT
I have gcc-5.4.0 installed separately on my Mac, so indeed that's what I 
compiled with.
Comment 10 Alexander Krotov 2016-10-20 16:57:22 EDT
> I have gcc-5.4.0 installed separately on my Mac, so indeed that's what I 
compiled with.

I just guess that Tommaso Pecorella compiled code with clang.

I think only clang generates error messages with such lines:
> ^~~~~~~~~~~~~~~~~~~

After reverting simulator.h, clang compiles everything in my case.
Comment 11 Tommaso Pecorella 2016-10-20 17:27:51 EDT
Yes, it's clang, as you can see from
"Apple LLVM version 8.0.0 (clang-800.0.38)"

What do you mean by reverting simulator.h ?


(In reply to Alexander Krotov from comment #10)
> > I have gcc-5.4.0 installed separately on my Mac, so indeed that's what I 
> compiled with.
> 
> I just guess that Tommaso Pecorella compiled code with clang.
> 
> I think only clang generates error messages with such lines:
> > ^~~~~~~~~~~~~~~~~~~
> 
> After reverting simulator.h, clang compiles everything in my case.
Comment 12 Alexander Krotov 2016-10-20 17:35:31 EDT
I just applied my patch and then did

hg revert -r 29bd2301b798 src/core/model/simulator.h

After that, everything compiles just fine. As for simulator.h, I am trying to understand where the bug is.
Comment 13 Alexander Krotov 2016-10-21 11:56:49 EDT
Created attachment 2633 [details]
Proposed patch

Updated patch, now it compiles with clang.
Comment 14 Alexander Krotov 2016-11-08 06:41:16 EST
Created attachment 2668 [details]
Proposed patch

Updated patch with MakeEvent implementation based on std::bind.

callback.h and timer-impl.h still contain lots of duplicate code, because they essentially implement currying (see SetArguments function that sets rest of the arguments when functor is already created) which is hard to make without C++14 support. I decided to leave them as is for now.
Comment 15 Peter Barnes 2017-01-05 17:30:35 EST
I'm almost done cleaning up some doxygen associated with this patch.  Apologies my "about a week" has turned in to about three months.

One question:  it's not obvious why both of these are needed:

template<typename... Ts>
static EventId 	Schedule (Time const &delay, Ts &&...args)
 
template<typename... Us, typename... Ts>
static EventId 	Schedule (Time const &delay, void(*f)(Us...), Ts &&...args)
Comment 16 Alexander Krotov 2017-01-05 18:07:31 EST
It should be easy to reproduce by removing the second definition. Basically, the problem was that compiler was unable to deduce function pointer type. I was able to solve the problem by specifying template types (had to add <void> after Schedule in some places), but to avoid changing existing code I provided more specific template.

As I understand it, the problem is that compiler cannot deduce function pointer type due to std::forward here:

return DoScheduleNow (MakeEvent (std::forward<Ts> (args)...));

Somewhat related:
https://stackoverflow.com/questions/7779900/why-is-template-argument-deduction-disabled-with-stdforward
Comment 17 Peter Barnes 2017-01-05 20:24:34 EST
Created attachment 2738 [details]
Revised doxygen

Revised patch, fixing up some doxygen issues.

This is based on r12434 02cdd3b19bdf from 2016-11-28, so might need updating...

Compiles with g++-5.4.0
Comment 18 Tommaso Pecorella 2017-01-06 06:43:18 EST
(In reply to Peter Barnes from comment #17)
> Created attachment 2738 [details]
> Revised doxygen
> 
> Revised patch, fixing up some doxygen issues.
> 
> This is based on r12434 02cdd3b19bdf from 2016-11-28, so might need
> updating...
> 
> Compiles with g++-5.4.0

Sorry... bad news.

Apple LLVM version 8.0.0 (clang-800.0.42.1)
Target: x86_64-apple-darwin15.6.0


../src/core/model/default-simulator-impl.cc:221:3: error: no matching function for call to 'Schedule'
  Simulator::Schedule<void ()> (delay, &Simulator::Stop);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/core/model/simulator.h:504:20: note: candidate function [with Ts = <void ()>] not viable: no overload of 'Stop' matching 'void (&&)()' for 2nd argument
EventId Simulator::Schedule (Time const &delay, Ts&&... args)
                   ^
../src/core/model/simulator.h:510:20: note: candidate function [with Us = <void ()>, Ts = <>] not viable: no overload of 'Stop' matching 'void (*)(void (*)())' for 2nd argument
EventId Simulator::Schedule (Time const &delay, void (*f)(Us...), Ts&&... args)
                   ^
1 error generated.
Comment 19 Alexander Krotov 2017-01-09 03:54:03 EST
That is what function pointer version is for. Instead of adding <void ()> (which should be <void> probably) I just provided two versions of Schedule. Otherwise changes are required to other modules, not only core.

I am going to merge all the patches and upload new version here.
Comment 20 Alexander Krotov 2017-01-20 09:58:22 EST
Created attachment 2751 [details]
Proposed patch

Updated patch, applies on top of the current tip (http://code.nsnam.org/ns-3-dev/rev/54fb5ab48d82). I think this one is ready to be pushed.
Comment 21 Stefano Avallone 2017-02-25 10:32:22 EST
Did you try to generate python bindings?

I am using variadic templates for the NetDeviceQueueInterface::NsLog function, but I am having troubles with the generation of python bindings:

In file included from /home/stefano/Tools/ns-3-dev-git/build/ns3/network-module.h:42,
                 from /home/stefano/Tools/ns-3-dev-git/src/network/bindings/scan-header.h:3:
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:202: error: expected nested-name-specifier before '...' token
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:202: error: expected '>' before '...' token
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:203: error: 'Args' has not been declared
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:203: error: expected `)' before 'args'
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:203: error: expected initializer before 'args'
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:375: error: expected nested-name-specifier before '...' token
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:375: error: expected '>' before '...' token
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:377: error: 'Args' has not been declared
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:377: error: expected `)' before 'args'
/home/stefano/Tools/ns-3-dev-git/build/ns3/net-device-queue-interface.h:377: error: expected initializer before 'args'

The above is the error message I get when running ./waf --apiscan=all.
Comment 22 natale.patriciello 2018-01-16 05:04:02 EST
Hello,

any news on that?
It would be wonderful to apply all this to ns-3 dev.
Comment 23 natale.patriciello 2018-01-16 05:04:15 EST
Hello,

any news on that?
It would be wonderful to apply all these to ns-3 dev.