On 09/02/2014 11:52 AM, Fabien COELHO wrote:
>
>> I've changed the loop slightly. Do you find this more readable than the way
>> the loop was previously written?
>
> It is 50% better:-)
>
> It is no big deal, but I still fail to find the remaining continue as
> usefull in this case. If you remove the "continue" line and invert the
> condition, it works exactly the same, so it is just one useless
> instruction within that loop. From a logical point of view the loop is
> looking for '%' and then check whether the next char is '%' or not, so the
> straightforward code helps my understanding as it does exactly that, and
> the continue is just an hindrance to comprehension.
>
> Note that I would buy it if it helped avoid indenting further a
> significant portion of complex code, but this is not the case here.
FWIW, I agree.
>> [doc] I've incorporated these changes into this version of the patch,
>> with small changes.
>
> Ok.
>
>> With elog(ERROR, ..) it's still reported, but the user isn't fooled into
>> thinking that the error is to be expected, and hopefully we would see a bug
>> report. If it's impossible to tell the two errors apart, we might have
>> subtly broken code carried around for who knows how long.
>
> Ok.
>
> In that case, it would make sense to keep distinct wordings of both
> exceptions in the execution code, so that they also can be set apart,
> i.e. keep the "too many/few" somewhere in the error?
Well, you can do "set log_error_verbosity='verbose'" if you run into that.
I think this patch has been thoroughly reviewed now. Committed, thanks!
- Heikki