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
______________________________________________
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
