Bugzilla – Full Text Bug Listing |
Summary: | Use variadic templates to support arbitrary number of arguments | ||
---|---|---|---|
Product: | ns-3 | Reporter: | Alexander Krotov <krotov> |
Component: | core | Assignee: | 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 |
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
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 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.
Created attachment 2630 [details]
Proposed patch
Updated patch to remove even more "ladders".
3.26 is out, how about pushing this patch?
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. 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 Hmm, I didnt' have errors using gcc-5.4.0 (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. I have gcc-5.4.0 installed separately on my Mac, so indeed that's what I compiled with. > 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. 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. 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. Created attachment 2633 [details]
Proposed patch
Updated patch, now it compiles with clang.
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.
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) 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 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
(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. 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. 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. 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. Hello, any news on that? It would be wonderful to apply all this to ns-3 dev. Hello, any news on that? It would be wonderful to apply all these to ns-3 dev. |
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.