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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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)