Bugzilla – Full Text Bug Listing |
Summary: | Add Time section of manual to avoid double usage. | ||
---|---|---|---|
Product: | ns-3 | Reporter: | natale.patriciello |
Component: | core | Assignee: | Peter Barnes <pdbarnes> |
Status: | PATCH PENDING --- | ||
Severity: | normal | CC: | bbojovic, mathieu.lacage, ns-bugs, tomh |
Priority: | P5 | ||
Version: | ns-3-dev | ||
Hardware: | PC | ||
OS: | Linux | ||
Attachments: | Patch |
Thank you Nat for writing this down. It is clearly explained. This is a good start, thanks. Some comments: 1) I would not advise to use auto in the example code. Almost no ns-3 code does it this way, and IMO it does not improve readability to use auto here. So in general, I would do the following: - auto t1 = ns3::MilliSeconds (2000); + Time t1 = MilliSeconds (2000); and also show the other prevalent usage pattern is equivalent: Time t1 (MilliSeconds (2000)); (note also dropping the 'ns3::'; what does it add here?) 2) There is another available example of floating point issues to add to your 'Seconds to integer' t4 case, which is a simple 'integer to Seconds' case: Time t5 = ns3::MilliSeconds (2); std::cout << t5.As (Time::S) << std::endl; yields +0.00199999999999999999s 3) suggest this editorial change: - IEEE 754 model is not precise (but no one in the world has come up with a better proposal) + IEEE 754 standard for floating point arithmetic has limits on the set of possible numbers that can be represented exactly. 4) Towards the end, it gives an example // auto t4 = ns3::Seconds (0.075); // Wrong auto t4 = ns3::MilliSeconds (75); // Right Again, the correct ns-3 way to store times is to use the Time class. The problem here is not the usage of the Time class, it is the usage of a floating point argument to set the Time value. Can you please clarify? 5) Can you please expand to recommend guidance for operators? I think that sometimes this is what induces the pattern of 'convert to double' usage that you are trying to avoid. e.g. times can be added and subtracted. Times can be multiplied and divided by integers, but Time * Time and Time/Time are not allowed. Users should avoid converting to double to perform arithmetic and then converting back to Time; instead try to find an operation using native Time operators. |
Created attachment 3195 [details] Patch As discussed in the mailing list, this is my proposal.