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

[Wacek Kusnierczyk]

Martin Maechler wrote:

[...]
>     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,
>   

what does ' "in principle" ' mean, as opposed to 'in principle'?  (is it
emphasis, or sneer quotes?)

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

of course, if the input *is* const and the output is expected to be
const, you should get an error/warning in the first case, and at least a
warning in the other (depending on the level of verbosity/pedanticity
you choose).

but my point was not to light-headedly change the signature/return of
elim_trailing and its implementation and use it in the original
context;  it was to either modify the context as well (if const is
inessential), or drop modifying the const string if the const is in fact
essential.


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

one way is to return from elim_trailing a new, const copy of the const
string.  using memcpy should be efficient enough.  care should be taken
to deallocate s when no longer needed.  (my guess is that using the
approach suggested here, s can be deallocated as soon as it is copied,
which means pretty much that it does not really have to be const.)

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

sure, we can discuss aesthetics offline.  as long as we do not discuss
aesthetics (do we?), it seems appropriate to me to keep the discussion
online.

i will experiment with a patch to solve this issue, and let you know
when i have something reasonable.

best,
vQ