On 18.04.2020 12:23, Julien Grall wrote: > On 30/03/2020 08:52, Jan Beulich wrote: >> On 28.03.2020 11:52, Julien Grall wrote: >>> On 26/03/2020 15:39, Jan Beulich wrote: >>>> On 22.03.2020 17:14, jul...@xen.org wrote: >>>>> @@ -3116,24 +3116,24 @@ int vcpu_destroy_pagetables(struct vcpu *v) >>>>> /* Free that page if non-zero */ >>>>> do { >>>>> - if ( mfn ) >>>>> + if ( !mfn_eq(mfn, _mfn(0)) ) >>>> >>>> I admit I'm not fully certain either, but at the first glance >>>> >>>> if ( mfn_x(mfn) ) >>>> >>>> would seem more in line with the original code to me (and then >>>> also elsewhere). >>> >>> It is doing *exactly* the same things. The whole point of typesafe >>> is to use typesafe helper not open-coding test everywhere. >>> >>> It is also easier to spot any use of MFN 0 within the code as you >>> know could grep "_mfn(0)". >>> >>> Therefore I will insist to the code as-is. >> >> What I insit on is that readability of the result of such changes be >> also kept in mind. The mfn_eq() construct is (I think) clearly less >> easy to read and recognize than the simpler alternative suggested. > > If mfn_eq() is less clear, then where do you draw the line when the > macro should or not be used?
I'm afraid there may not be a clear line to draw until everything got converted. I do seem to recall though that, perhaps in a different context, Andrew recently agreed with my view here (Andrew, please correct me if I'm wrong). It being a fuzzy thing, I guess maintainers get to judge ... Jan