Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@gmail.com> wrote:
> On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx