[GIT] ppl/ppl(master): Magic constants avoided.
Module: ppl/ppl Branch: master Commit: a52e104fe997e8347139bc14cd6d1c44f40f9d08 URL: http://www.cs.unipr.it/git/gitweb.cgi?p=ppl/ppl.git;a=commit;h=a52e104fe997e...
Author: Roberto Bagnara bagnara@cs.unipr.it Date: Sun Oct 9 19:33:37 2011 +0200
Magic constants avoided. Detected by ECLAIR service nomagicc.
---
Watchdog/src/Time.cc | 2 +- Watchdog/src/Time.defs.hh | 6 ++++++ Watchdog/src/Time.inlines.hh | 17 +++++++++-------- 3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/Watchdog/src/Time.cc b/Watchdog/src/Time.cc index 9b52e4b..afc3aaf 100644 --- a/Watchdog/src/Time.cc +++ b/Watchdog/src/Time.cc @@ -29,5 +29,5 @@ namespace PWL = Parma_Watchdog_Library;
bool PWL::Time::OK() const { - return microsecs < 1000000; + return microsecs < MUSECS_IN_SEC; } diff --git a/Watchdog/src/Time.defs.hh b/Watchdog/src/Time.defs.hh index 003d07c..3fd73fc 100644 --- a/Watchdog/src/Time.defs.hh +++ b/Watchdog/src/Time.defs.hh @@ -100,6 +100,12 @@ public: bool OK() const;
private: + //! Number of microseconds in a second. + static const unsigned long MUSECS_IN_SEC = 1000000UL; + + //! Number of hundredths of a second in a second. + static const unsigned long HSECS_IN_SEC = 100UL; + //! Number of seconds. unsigned long secs;
diff --git a/Watchdog/src/Time.inlines.hh b/Watchdog/src/Time.inlines.hh index cfcc814..7b005ac 100644 --- a/Watchdog/src/Time.inlines.hh +++ b/Watchdog/src/Time.inlines.hh @@ -36,8 +36,9 @@ Time::Time()
inline Time::Time(unsigned long hundredths_of_a_second) - : secs(hundredths_of_a_second / 100), - microsecs((hundredths_of_a_second % 100) * 10000) { + : secs(hundredths_of_a_second / HSECS_IN_SEC), + microsecs((hundredths_of_a_second % HSECS_IN_SEC) + * (MUSECS_IN_SEC/HSECS_IN_SEC)) { assert(OK()); }
@@ -45,9 +46,9 @@ inline Time::Time(unsigned long s, unsigned long m) : secs(s), microsecs(m) { - if (microsecs >= 1000000) { - secs += microsecs / 1000000; - microsecs %= 1000000; + if (microsecs >= MUSECS_IN_SEC) { + secs += microsecs / MUSECS_IN_SEC; + microsecs %= MUSECS_IN_SEC; } assert(OK()); } @@ -66,9 +67,9 @@ inline Time& Time::operator+=(const Time& y) { unsigned long r_secs = secs + y.secs; unsigned long r_microsecs = microsecs + y.microsecs; - if (r_microsecs >= 1000000) { + if (r_microsecs >= MUSECS_IN_SEC) { ++r_secs; - r_microsecs %= 1000000; + r_microsecs %= MUSECS_IN_SEC; } secs = r_secs; microsecs = r_microsecs; @@ -82,7 +83,7 @@ Time::operator-=(const Time& y) { long r_microsecs = microsecs - y.microsecs; if (r_microsecs < 0) { --r_secs; - r_microsecs += 1000000; + r_microsecs += MUSECS_IN_SEC; } if (r_secs < 0) r_secs = r_microsecs = 0;
Roberto --
On Sun, Oct 9, 2011 at 12:59 PM, Roberto Bagnara bagnara@cs.unipr.itwrote:
- //! Number of microseconds in a second.
- static const unsigned long MUSECS_IN_SEC = 1000000UL;
There is a long history of using just "u" (or "U") as the ASCII-7 representation for "mu" when used as a prefix for 1e-6.
E.g., tv_sec and tv_usec in the "timeval" struct: http://pubs.opengroup.org/onlinepubs/009604599/basedefs/sys/time.h.html
+
- //! Number of hundredths of a second in a second.
- static const unsigned long HSECS_IN_SEC = 100UL;
It would be somewhat more consistent (and SI-similar) to use "CSECS_IN..."; "centi-" is 1e-2, while "hecta-" is 1e+2.
Both entirely trivial, but they caught my eye.
Also, I wonder if "_PER_" would read more clearly than "_IN_"? I tend towards the former in my code, but I don't know how much of a taste issue that is. As a native [american] english speaker, I sometimes read "x in y" as "x in terms of y", while "x per y" is less ambiguous.
Best regards, Anthony Foiani
On 10/10/11 06:03, Anthony Foiani wrote:
On Sun, Oct 9, 2011 at 12:59 PM, Roberto Bagnara <bagnara@cs.unipr.it mailto:bagnara@cs.unipr.it> wrote:
+ //! Number of microseconds in a second. + static const unsigned long MUSECS_IN_SEC = 1000000UL;There is a long history of using just "u" (or "U") as the ASCII-7 representation for "mu" when used as a prefix for 1e-6.
E.g., tv_sec and tv_usec in the "timeval" struct: http://pubs.opengroup.org/onlinepubs/009604599/basedefs/sys/time.h.html
+ + //! Number of hundredths of a second in a second. + static const unsigned long HSECS_IN_SEC = 100UL;It would be somewhat more consistent (and SI-similar) to use "CSECS_IN..."; "centi-" is 1e-2, while "hecta-" is 1e+2.
Both entirely trivial, but they caught my eye.
Also, I wonder if "_PER_" would read more clearly than "_IN_"? I tend towards the former in my code, but I don't know how much of a taste issue that is. As a native [american] english speaker, I sometimes read "x in y" as "x in terms of y", while "x per y" is less ambiguous.
Hi Anthony,
thank you very much for your report. I believe you are completely right: I have made the necessary changes in commit http://www.cs.unipr.it/git/gitweb.cgi?p=ppl/ppl.git;a=commit;h=8f37574794416... Cordially,
Roberto
Roberto --
On Mon, Oct 10, 2011 at 12:46 AM, Roberto Bagnara bagnara@cs.unipr.itwrote:
On 10/10/11 06:03, Anthony Foiani wrote:
On Sun, Oct 9, 2011 at 12:59 PM, Roberto Bagnara <bagnara@cs.unipr.it<mailto:
bagnara@cs.unipr.it>> wrote:
There is a long history of using just "u" (or "U") as the ASCII-7 representation for "mu" when used as a prefix for 1e-6.
It would be somewhat more consistent (and SI-similar) to use "CSECS_IN..."; "centi-" is 1e-2, while "hecta-" is 1e+2.
Both entirely trivial, but they caught my eye.
Also, I wonder if "_PER_" would read more clearly than "_IN_"? I tend towards the former in my code, but I don't know how much of a taste issue that is. As a native [american] english speaker, I sometimes read "x in y" as "x in terms of y", while "x per y" is less ambiguous.
Hi Anthony,
thank you very much for your report. I believe you are completely right: I have made the necessary changes in commit http://www.cs.unipr.it/git/**gitweb.cgi?p=ppl/ppl.git;a=**commit;h=** 8f37574794416e4c97d012e8a51be4**2fe3b310aahttp://www.cs.unipr.it/git/gitweb.cgi?p=ppl/ppl.git;a=commit;h=8f37574794416e4c97d012e8a51be42fe3b310aa
That's more of a delta than I expected! :) Glad you like the idea.
A quick look through the diff offers a few more possible changes:
1. Watchdog/tests/watchdog1.cc
Replace "100.0" with an appropriate cast of CSEC_PER_SECOND?
2. demos/ppl_lpsol/ppl_lpsol.c
I had to stare at it for a while, but I think that you could possibly replace most of that chunk with:
int secs = current_secs - saved_secs; int usecs = current_usecs - saved_usecs; if ( usecs < 0 ) { secs += 1; usecs += USECS_PER_SEC; } assert( secs >= 0 && usecs >= 0 ); int csec = ( usecs + USECS_PER_HALF_CSEC ) / USEC_PER_CSEC; fprintf( f, "%d.%02d", secs, csec );
That conversion is a bit messy; if you really want to kill all the "magic constants", consider providing a conversion function (e.g., nearest_csec_to_usec(...)?) Possibly massive overkill.
(And to be POSIX/SUS correct, I believe that we really want "suseconds_t" for usecs there, not "int" -- but I have no idea if that really matters, or if it'd just cause pain on not-perfectly-current platforms...)
(And if we're being verbose, I'd be tempted to go with "elapsed_secs", "elapsed_usecs", and "elapsed_csecs", instead of just "secs", "usecs", and "csecs".)
3. interfaces/Java/jni/ppl_java_globals.cc
It's not clear that the second "assert" can ever trigger if the first one doesn't. But maybe jtype_to_unsigned does bad things...
4. (various interfaces, at least OCaml and Prolog) "some time after ... after that call"
That reads a little awkwardly. Maybe: "Computations taking exponential time will be attempted for at least <CODE>csecs</CODE> centiseconds; if a solution has not been found, a timeout exception will be thrown." Or something along those lines?
5. utils/timings.cc
A few more "magic constants" to squash. Or possibly replace by the simpler conditional described in (1) above. Maybe a helper function? format_time_delta(...)?
No matter what, these are all very trivial, and can easily be ignored.
Thanks again for the great library!
Best regards, Anthony Foiani
participants (2)
-
Anthony Foiani -
Roberto Bagnara