>>>>> "vQ" == Wacek Kusnierczyk <waclaw.marcin.kusnierc...@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
______________________________________________
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
