On 26.04.2024 14:09, Oleksii wrote: > On Fri, 2024-04-26 at 12:51 +0200, Jan Beulich wrote: >> On 26.04.2024 10:21, Oleksii wrote: >>> On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote: >>>> On 17.04.2024 12:04, Oleksii Kurochko wrote: >>>>> Return type was left 'int' because of the following compilation >>>>> error: >>>>> >>>>> ./include/xen/kernel.h:18:21: error: comparison of distinct >>>>> pointer >>>>> types lacks a cast [-Werror] >>>>> 18 | (void) (&_x == &_y); \ >>>>> | ^~ >>>>> common/page_alloc.c:1843:34: note: in expansion of macro >>>>> 'min' >>>>> 1843 | unsigned int inc_order = min(MAX_ORDER, >>>>> flsl(e >>>>> - s) - 1); >>>> >>>> Apart from this I'm okay with this patch, assuming Andrew's won't >>>> change in >>>> any conflicting way. As to the above - no, I don't see us having >>>> ffs() / ffsl() >>>> returning unsigned int, fls() / flsl() returning plain int. Even >>>> more >>>> so that, >>>> given the LHS variable's type, an unsigned quantity is really >>>> meant >>>> in the >>>> quoted code. >>> If I understand you correctly, it's acceptable for fls() / flsl() >>> to >>> return 'int'. Therefore, I can update the commit message by >>> removing >>> the part mentioning the compilation error, as it's expected for >>> fls() / >>> flsl() to return 'int'. Is my understanding correct? >> >> No. I firmly object to ffs() and fls() being different in their >> return >> types. I'm sorry, I realize now that my earlier wording was ambiguous >> (at least missing "but" after the comma). > Thanks for clarifying. > > I can change return type of fls() / flsl() to 'unsingned int' to be the > same as return type of ffs() / ffsl(), but then it will be needed to > add a cast in two places:
Except that no, it doesn't really need casts there. > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1842,7 +1842,7 @@ static void _init_heap_pages(const struct > page_info *pg, > * Note that the value of ffsl() and flsl() starts from 1 > so we need > * to decrement it by 1. > */ > - unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1); > + unsigned int inc_order = min((unsigned int)MAX_ORDER, > flsl(e - s) - 1); The preferred course of action would want to be to simply make MAX_ORDER expand to an unsigned constant. Depending on the amount of fallout, an alternative would be to use _AC(MAX_ORDER, U) here. Yet another alternative would be to use MAX_ORDER + 0U here, as iirc we do in a few other places, for similar purposes. Avoiding a cast here is not only shorter, but - see statements elsewhere - generally preferable. Jan
