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

[Martin Maechler]

Hi Waclav (and other interested parties),

I have committed my working version of src/main/coerce.c
so you can prepare your patch against that.

Thank you in advance!
Martin

On Fri, May 29, 2009 at 21:54, Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no> wrote:
> 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
>
>