On Thu, 08 Feb 2018, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Thu, Feb 08, 2018 at 05:13:02PM +0200, Mika Kuoppala wrote: >> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: >> >> > Quoting Tvrtko Ursulin (2018-02-08 14:34:38) >> >> >> >> On 08/02/2018 14:22, Ville Syrjälä wrote: >> >> > On Thu, Feb 08, 2018 at 01:06:05PM +0000, Tvrtko Ursulin wrote: >> >> >> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> >> >> >> >> >> >> Coccinelle transformation: >> >> >> >> >> >> @@ >> >> >> expression p, g; >> >> >> @@ >> >> >> ( >> >> >> -INTEL_GEN(p) > g >> >> >> +IS_GEN_GT(p, g) >> >> > >> >> > I think this stuff makes the code pretty close to illegible. >> >> > In this particular case even more so because "GT" actually >> >> > means something very different to us. >> >> >> >> Oh how true! And I did not realize it at all while writing it! :) >> >> >> >> Anyway, something like this, regardless of a name, is needed if people >> >> want this to be effective. Since the checks have to be moved to known at >> >> compile time. Or a completely different approach will be needed. >> > >> > IS_GEN_RANGE() doesn't cut it? >> > >> >> IS_GEN_RANGE(8,9); >> >> short and readable > > 'if (IS_GEN_RANGE(...))' reads funny. IS_GEN_IN_RANGE() would be more > englishy perhaps, but it looks a bit off to me for whatever reason. We already have IS_GEN(dev_priv, start, end) for inclusive ranges with GEN_FOREVER as unbound start/end. There's no need to bikeshed the naming further. (Except perhaps the GEN_FOREVER part.) With some macro vararg hacking, we could probably make that work for IS_GEN(dev_priv, exact) too, to replace say IS_GEN5() with one macro if we like. Just to make more code use similar constructs. > And it still doesn't tell you anything about inclusive vs. exlusive. > So it just forces you to waste brain cells on mundane details when > reading the code. IMO that's a fairly bad tradeoff. Agreed. I think this patch is by far the worst part in the series. Especially the _GT, _GTE, _LT, _LTE macros are IMO unacceptable. Quick greping shows that we have much more inclusive than exclusive range checks. We could help our brains a bit by switching, uh, exclusively to inclusive ranges. That's not perfect, by far, as some checks naturally work better with < and >, but I think I'd rather have that with IS_GEN() than a bunch of macros you have to stop to think about. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
