Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

Hi Jan,

> > > > > While I've committed this patch (hoping that I got the necessary
> > > > > context
> > > > > adjustment right for the
> > > > > automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > change), I'd like to come back to this before going further with 
> > > > > users
> > > > > of
> > > > > the new macro: I still think we ought to try to get to the single
> > > > > evaluation wherever possible. The macro would then be used only in
> > > > > cases
> > > > > where the alternative construct (perhaps an isolate_lsb() macro, 
> > > > > living
> > > > > perhaps in xen/bitops.h) cannot be used. ISOLATE_LSB() would then 
> > > > > want
> > > > > to
> > > > > gain a comment directing people to the "better" sibling. Thoughts?
> > > > Having the users in place would help me estimate the remaining work 
> > > > that
> > > > needs to be done on this rule and see if my local counts match up 
> > > > with
> > > > the counts in staging.
> > > By "having the users in place", you mean you want other patches in 
> > > this
> > > and the dependent series to be committed as-is (except for the name
> > > change)? That's what I'd like to avoid, as it would mean touching all
> > > those use sites again where the proposed isolate_lsb() could be used
> > > instead. I'd rather see all use sites be put into their final shape
> > > right away.
> > This request is coming a bit late and also after all the patches have
> > been reviewed already. I for one am not looking forward to review them
> > again.
> >
> > That said, if you could be more specified maybe it could become
> > actionable:
> >
> > - do you have a pseudo code implementation of the "better" macro you
> >   would like to propose?
> May I remind you that I made this request (including a draft 
> implementation)
> before already, and Nicola then merely found that the evaluate-once 
> form
> simply cannot be used everywhere? Anybody could have thought of the 
> option
> of "splitting" the macro. After all I hope that there is no 
> disagreement on
> macro arguments better being evaluated just once, whenever possible.
>
> > - do you have an list of call sites you would like to be changed to 
> > use
> >   the "better" macro?
> No, I don't have a list. But the pattern is pretty clear: The "better" 
> form
> ought to be used wherever it actually can be used.
>
> > Also, you might remember past discussions about time spent making
> > changes yourself vs. others doing the same. This is one of those cases
> > that it would be faster for you to make the change and send a patch 
> > than
> > explaining someone else how to do it, then review the result (and
> > review it again as it probably won't be exactly as you asked the first
> > time.)
> >
> > If you don't want the call sites to be changes twice, may I suggest 
> > you
> > provide a patch on top of Nicola's series, I review and ack your 
> > patch,
> > and Nicola or I rebase & resend the series so that the call sites are
> > only changes once as you would like? I think that's going to be way
> > faster.
> I'll see if I can find time to do so. I don't normally work on top of
> other people's uncommitted patches, though ... So I may also choose to 
> go
> a slightly different route. (You realize though that all still pending
> patches using the new macro need touching again anyway, don't you?)
>
> Jan
Then perhaps it's best if I give it a try at doing the single evaluation 
macro, so that I can make a series modifying the call sites only once on 
top of that and send everything in one go. Before doing that, though, 
I'll make a thread where various aspects that are not so clear yet can 
be discussed, so that we can devise a robust solution (also to dig this 
out of this deep thread).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)