On Fri, Mar 01, 2024 at 09:56:41PM +0300, Nikita Kiryushin wrote: > On 2/29/24 15:30, Ville Syrjälä wrote: > > I prefer the current way where we have no side effects in > > the if statement. > > > > This seem like a valid concern from readability and maintainability > standpoint. My patch was aimed mostly at performance and maintainability > using tools: some more pedantic analyzers are sensitive to non-checked > return values (as of now, drm_rect_intersect is ignored). > > Would it be a better idea to make an update to the patch with second > drm_rect_visible call changed to an appropriately named state flag set > with drm_rect_intersect result? I was thinking of maybe removing that drm_rect_visible() from drm_rect_intersect() entirely, but looks like it's used fairly extensively, so would require a bunch of work. But now that I though about this I recalled that there was an earlier patch trying to do exactly what you suggested in this patch. And looks like there was a second version posted which I completely missed: https://patchwork.freedesktop.org/series/115605/ While that does still have drm_rect_intersect() with its side effects inside the if() I don't find it quite as objectionable since it's the only thing in there. So it's a bit more obvious what is happening. I've gone and merged that one. Thanks for the patch regardless. At least I reminded me to look at the earlier attempt ;) > > BTW, the original patch somehow got mangled while it made its way to the > patchwork: source list line in patch got broken, which permits the patch > from being applied (the original version did not have that line break). > Any ideas how to prevent this happening with the second version of patch > (in case the idea is viable)? -- Ville Syrjälä Intel
