[PPL-devel] [GIT] ppl/ppl(master): Magic constants avoided.

[Anthony Foiani]

Roberto --

On Mon, Oct 10, 2011 at 12:46 AM, Roberto Bagnara <bagnara at cs.unipr.it>wrote:

> On 10/10/11 06:03, Anthony Foiani wrote:
>
>  On Sun, Oct 9, 2011 at 12:59 PM, Roberto Bagnara <bagnara at cs.unipr.it<mailto:
>> bagnara at 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**2fe3b310aa<http://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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.cs.unipr.it/pipermail/ppl-devel/attachments/20111010/2d508f2c/attachment.htm>