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

[Wacek Kusnierczyk]

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.
>
>   

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)
>   

the first argument is const char*, which usually means a contract
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++;
>   

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

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

likewise

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

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

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

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

was perhaps

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

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

one way out is to drop the violated const in both the actual argument
and in elim_trailing, which would then be simplified by removing all
const qualifiers and (char*) casts.  another way out is to make
elim_trailing actually allocate and return a new string, keeping the
input truly constant, at a performance cost    .  yet another way is to
ignore the issue, of course.

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


vQ