Hi Cédric,

On Wed, Nov 06, 2019 at 06:38:53PM +0100, Cédric Dufour wrote:
> [sorry for mis-threading; hoping I got the git send-mail --in-reply-to right]

Don't worry about this. The purpose of the list is precisely to act as
humans and not as pre-configured bots. Whatever you can do right which
saves others' time is fine. What you can't easily do is left to others,
that's how we're the most efficient.

> Actually, reading through your original and last comments, I realize I must
> have misunderstood the sample_expr() part and got carried away.
> 
> Unless I'm mistaken, we can use the existing sample_fetch_as_type() function
> directly (without any further addition to sample.c). Or am I missing 
> something ?

Initially when I quickly had a look at it I had the impression that it
would only process a sample fetch function but not the whole expression.
That disturbed me a little bit because I thought we had something to do
this. Now I had a second look after your comment and I think you're
right :-)

> This leaves the switch(rule->from) <-> smpt_opt_dir stuff. Since there is
> nothing about act_rule in sample.c so I felt it has no place there.

Absolutely.

> Wouldn't an extra function call just to deal with this switch() be overkill ?

Yes I think it would be. Let's just place your switch() where you need it
and simply rely on sample_fetch_as_type() to do most of the job. I don't
see what could cause trouble there. Oh I'm seeing that it's apparently
what you did in this new version of the patch. I reviewed it very quickly
but at first glance it looks OK.

> Let me know if you still think this ought to go in a separate function
> (like if anticipating set_gpt1 :-) ).

Not now, it looks OK as-is.

Just let me know if you want me to merge this one now or if you made some
extra changes since you posted.

Thanks,
Willy

Reply via email to