From maechler at stat.math.ethz.ch  Fri May 29 23:48:30 2009
From: maechler at stat.math.ethz.ch (Martin Maechler)
Date: Fri, 29 May 2009 23:48:30 +0200
Subject: [Rd] as.numeric(levels(factor(x))) may be a decreasing sequence
In-Reply-To: <4A203D67.8000007@idi.ntnu.no>
References: <20090523074454.GA17112@cs.cas.cz>
	<18973.42970.826686.173830@cmath-5.math.ethz.ch>
	<4A1DC057.80401@idi.ntnu.no>
	<18975.59582.885300.549159@lynne.math.ethz.ch>
	<4A203D67.8000007@idi.ntnu.no>
Message-ID: <3f1d2410905291448y271fa484te82b8dbaa547017f@mail.gmail.com>

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



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