On 29.06.2023 21:20, Stefano Stabellini wrote: > On Thu, 29 Jun 2023, Luca Fancellu wrote: >>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetr...@bugseng.com> wrote: >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int >>> *fl, int *sl) >>> *fl = flsl(*r) - 1; >>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>> *fl -= FLI_OFFSET; >>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> - *fl = *sl = 0; >>> - */ >>> + ASSERT( *fl >= 0 ); >> >> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it >> with spaces >> before and after the condition (like our if conditions) so I think they can >> be dropped. > > Yes, that's right. I am OK with this patch but I think we should wait > for Jan's ack to be sure. > > An alternative that I feel more comfortable in Acking myself because it > doesn't change the semantics of this code would be to change the 3 lines > of code above with this: > > /* > * ; FL will be always >0! > * if ((*fl -= FLI_OFFSET) < 0) > * fl = *sl = 0; > */
While I'd be okay with this form, as Luca says it'll get us a different violation, which we ought to avoid. While I was the one to suggest the conversion to ASSERT(), having thought about it yet once more I'm now of the opinion that _any_ transformation of this commented out piece of code needs first understanding what was originally meant. Or alternatively, while converting to #if form, to add a comment making crystal clear that it's simply uncertain what was meant. Jan
