[Rd] as.numeric(levels(factor(x))) may be a decreasing sequence

[Martin Maechler]

>>>>> "vQ" == Wacek Kusnierczyk <Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>     on Thu, 28 May 2009 00:36:07 +0200 writes:

    vQ> Martin Maechler wrote:
    >> 
    >> I have very slightly  modified the changes (to get rid of -Wall
    >> warnings) and also exported the function as Rf_dropTrailing0(),
    >> and tested the result with 'make check-all' .
    >> As the change seems reasonable and consequent, and as
    >> it seems not to produce any problems in our tests, 
    >> I'm hereby proposing to commit it (my version of it),
    >> [to R-devel only] within a few days, unless someone speaks up.



    vQ> i may be misunderstanding the code, but:


    >> Martin Maechler, ETH Zurich
    >> 
    PS> --- R-devel/src/main/coerce.c	2009-04-17 17:53:35.000000000 +0200
    PS> +++ R-devel-elim-trailing/src/main/coerce.c	2009-05-23 08:39:03.914774176 +0200
    PS> @@ -294,12 +294,33 @@
    PS> else return mkChar(EncodeInteger(x, w));
    PS> }
    >> 
    PS> +const char *elim_trailing(const char *s, char cdec)
    >> 

    vQ> the first argument is const char*, which usually means a contract
    vQ> promising not to change the content of the pointed-to object

    PS> +{
    PS> +    const char *p;
    PS> +    char *replace;
    PS> +    for (p = s; *p; p++) {
    PS> +        if (*p == cdec) {
    PS> +            replace = (char *) p++;

  vQ> const char* p is cast to non-const char* replace

    PS> +            while ('0' <= *p & *p <= '9') {
    PS> +                if (*(p++) != '0') {
    PS> +                    replace = (char *) p;

  vQ> likewise

    PS> +                }
    PS> +            }
    PS> +            while (*(replace++) = *(p++)) {
    >> 

    vQ> the char* replace is assigned to -- effectively, the content of the
    vQ> promised-to-be-constant string s is modified, and the modification may
    vQ> involve any character in the string.  (it's a no-compile-error contract
    vQ> violation;  not an uncommon pattern, but not good practice either.)

    PS> +                ;
    PS> +            }
    PS> +            break;
    PS> +        }
    PS> +    }
    PS> +    return s;
    >> 

    vQ> you return s, which should be the same pointer value (given the actual
    vQ> code that does not modify the local variable s) with the same pointed-to
    vQ> string value (given the signature of the function).

    vQ> was perhaps

    vQ> char *elim_trailing(char* const s, char cdec)

    vQ> intended?

yes that would seem slightly more "logical" to my eyes, 
and "in principle" I also agree with the other remarks you make above,
...

    vQ> anyway, having the pointer s itself declared as const does
    vQ> make sense, as the code seems to assume that exactly the input pointer
    vQ> value should be returned.  or maybe the argument to elim_trailing should
    vQ> not be declared as const, since elim_trailing violates the declaration. 

    vQ> one way out is to drop the violated const in both the actual argument
    vQ> and in elim_trailing, which would then be simplified by removing all
    vQ> const qualifiers and (char*) casts.  

I've tried that, but   ``it does not work'' later:
{after having renamed  'elim_trailing'  to  'dropTrailing0' }
my version of *using* the function was

1 SEXP attribute_hidden StringFromReal(double x, int *warn)
2 {
3   int w, d, e;
4   formatReal(&x, 1, &w, &d, &e, 0);
5   if (ISNA(x)) return NA_STRING;
6   else return mkChar(dropTrailing0(EncodeReal(x, w, d, e, OutDec), OutDec));
7 }

where you need to consider that mkChar() expects a 'const char*' 
and EncodeReal(.) returns one, and I am pretty sure this was the
main reason why Petr had used the two 'const char*' in (the
now-named) dropTrailing0() definition. 
If I use your proposed signature

char* dropTrailing0(char *s, char cdec);

line 6 above gives warnings in all of several incantations I've tried
including this one :

    else return mkChar((const char *) dropTrailing0((char *)EncodeReal(x, w, d, e, OutDec), OutDec));

which (the warnings) leave me somewhat clue-less or rather
unmotivated to dig further, though I must say that I'm not the
expert on the subject char*  / const char* ..

    vQ>   another way out is to make
    vQ> elim_trailing actually allocate and return a new string, keeping the
    vQ> input truly constant, at a performance cost    .  yet another way is to
    vQ> ignore the issue, of course.

    vQ> the original (martin/petr) version may quietly pass -Wall, but the
    vQ> compiler would complain (rightfully) with -Wcast-qual.

hmm, yes, but actually I haven't found a solution along your
proposition that even passes   -pedantic -Wall -Wcast-align
(the combination I've personally been using for a long time).

Maybe we can try to solve this more esthetically
in private e-mail exchange?

Regards,
Martin