On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@xxxxxxxxx> wrote: > On 19 March 2018 at 18:17, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: >> Quoting Matthew Auld (2018-03-19 18:08:54) >>> GEM_WARN_ON() was originally intended to be used only as: >>> >>> if (GEM_WARN_ON(expr)) >>> ... >>> >>> but it just so happens to also work as simply: >>> >>> GEM_WARN_ON(expr); >>> >>> since it just wraps WARN_ON, which is a little misleading since for >>> !DRM_I915_DEBUG_GEM builds the second case will actually break the >>> build. Given that there are some patches floating around which seem to >>> miss this, it probably makes sense to just make it work for both cases. >> >> That really was quite intentional. The only time to use GEM_WARN_ON() is >> inside an if, otherwise what's the point? > > Why wouldn't we want it to behave like WARN_ON? That seems to be what > people expect, since it does wrap WARN_ON, and we don't always use > WARN_ON in an if... Looking at this, I'm more baffled by GEM_WARN_ON() evaluating to expr on CONFIG_DRM_I915_DEBUG_GEM=y and 0 otherwise. That's the seriously misleading part here. Are you sure all those if (GEM_WARN_ON(expr)) are to be ignored? I'm no expert on gem code, but I could be easily persuaded to believe not. BR, Jani. PS. On the original question, if you design GEM_WARN_ON() to be used within if conditions only, I think you better squeeze in an inline function with __must_check. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
