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
