On 15/12/16 16:51, Laurent Pinchart wrote: >>> + WARN_ON(drm_crtc_vblank_get(crtc) != 0); >> >> I don't like this style very much. I think WARN()s should be without >> side effects. >> >> Also, WARN only gives benefit when we don't know what the call stack is. >> Afaik, there's only one way omap_crtc_atomic_flush can be called, so >> it's just extra spam and dev_err or dev_warn should be enough. > > I've used it because the equivalent statements testing omap_crtc- >> vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more vocal, > it really gets the point across. As the function really should not return an > error unless in case of a driver bug, I don't think it will generate any spam. > I don't care too much though, I can replace it with a dev_err() if you insist. I don't mind using WARN_ON() that much. But that's the comment I've received a few times, so I shared it =). What I do care is the side effect inside WARN_ON. At least for me it's quite easy to miss that it's actually having a side effect, as I expect WARN_ON() to be just a check, like assert(). So when reading the code, I skip the WARN_ONs. Tomi
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
