From mboxrd@z Thu Jan 1 00:00:00 1970 From: NeilBrown Subject: Re: [PATCH] kernel.h: Add for_each_if() Date: Tue, 10 Jul 2018 20:32:23 +1000 Message-ID: <871scbwfd4.fsf@notabene.neil.brown.name> References: <20180709083650.23549-1-daniel.vetter@ffwll.ch> <20180709162509.29343-1-daniel.vetter@ffwll.ch> <20180709163001.8fb8148223a57bc46a13fbda@linux-foundation.org> <20180710075328.GG3008@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Return-path: In-Reply-To: <20180710075328.GG3008@phenom.ffwll.local> Sender: linux-kernel-owner@vger.kernel.org To: Daniel Vetter , Andrew Morton Cc: Daniel Vetter , LKML , DRI Development , Intel Graphics Development , Gustavo Padovan , Maarten Lankhorst , Sean Paul , David Airlie , Kees Cook , Ingo Molnar , Greg Kroah-Hartman , Wei Wang , Stefan Agner , Andrei Vagin , Randy Dunlap , Andy Shevchenko , Yisheng Xie , Peter Zijlstra , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Tue, Jul 10 2018, Daniel Vetter wrote: > On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote: >> On Mon, 9 Jul 2018 18:25:09 +0200 Daniel Vetter wrote: >>=20 >> > To avoid compilers complainig about ambigious else blocks when putting >> > an if condition into a for_each macro one needs to invert the >> > condition and add a dummy else. We have a nice little convenience >> > macro for that in drm headers, let's move it out. Subsequent patches >> > will roll it out to other places. >> >=20 >> > The issue the compilers complain about are nested if with an else >> > block and no {} to disambiguate which if the else belongs to. The C >> > standard is clear, but in practice people forget: >> >=20 >> > if (foo) >> > if (bar) >> > /* something */ >> > else >> > /* something else >>=20 >> um, yeah, don't do that. Kernel coding style is very much to do >>=20 >> if (foo) { >> if (bar) >> /* something */ >> else >> /* something else >> } >>=20 >> And if not doing that generates a warning then, well, do that. >>=20 >> > The same can happen in a for_each macro when it also contains an if >> > condition at the end, except the compiler message is now really >> > confusing since there's only 1 if: >> >=20 >> > for_each_something() >> > if (bar) >> > /* something */ >> > else >> > /* something else >> >=20 >> > The for_each_if() macro, by inverting the condition and adding an >> > else, avoids the compiler warning. >>=20 >> Ditto. >>=20 >> > Motivated by a discussion with Andy and Yisheng, who want to add >> > another for_each_macro which would benefit from for_each_if() instead >> > of hand-rolling it. >>=20 >> Ditto. >>=20 >> > v2: Explain a bit better what this is good for, after the discussion >> > with Peter Z. >>=20 >> Presumably the above was discussed in whatever-thread-that-was. > > So there's a bunch of open coded versions of this already in kernel > headers (at least the ones I've found). Not counting the big pile of > existing users in drm. They are all wrong and should be reverted to a > plain if? That why there's a bunch more patches in this series. > > And yes I made it clear in the discussion that if you sprinkle enough {} > there's no warning, should have probably captured this here. > > Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I > can stop bothering with this. I think is it problematic to have macros like #define for_each_foo(...) for (......) if (....) because for_each_foo(...) if (x) ....; else ......; is handled badly. So in that sense, your work seems like a good thing. However it isn't clear to me that you need a new macro. The above macro could simply be changed to #define for_each_foo(...) for (......) if (!....);else Clearly people don't always think to do this, but would adding a macro help people to think? If we were to have a macro, it isn't clear to me that for_each_if() is a good name. Every other macro I've seen that starts "for_each_" causes the body to loop. This one doesn't. If someone doesn't know what for_each_if() does and sees it in code, they are unlikely to jump to the right conclusion. I would suggest that "__if" would be a better choice. I think most people would guess that means "like 'if', but a bit different", which is fairly accurate. I think the only sure way to avoid bad macros being written is to teach some static checker to warn about any macro with a dangling "if". Possibly checkpatch.pl could do that (but I'm not volunteering). I do agree that it would be good to do something, and if people find for_each_fi() to actually reduce the number of poorly written macros, then I don't object to it. Thanks, NeilBrown --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAltEizcACgkQOeye3VZi gbkIRA/9Gdq2PO/PLeUxQ1OV2giyRABxX/KIlSN+6Yi9WdFxz2l1v+8ea5CSjT4W ZK7iRRGzsKTFfZKl5EoaxgHG4iHsI3dBgZVmc3YKD1BI9xOyBoClw2hTaDSw8awo Sgbmq5I0mV+dsSF4FuzkbfaZ923LwIqVlcztAWl7U+M1ISds2Ou03NlT45iSwWhU mRPuPfYTI9bciPcYaaTRkmSacjKdvCtzUZmbhT62nidy42a/fquKLNhv/s5D4hlt TyeArWMFdeKDwr255ffglMKiaMNBFUrEElhvFRyBnAgWI/tEVhte5Hu+MZbtboG5 t2DKblGUpc8VpDlPsUlP75SntsTmtj5Rz6KA8EVWoVpjQxeaieBkBNowPiWDdvRx COlSytBaFRBxIqWSMNK/wzMZ6fLHkJy0WS+9YgAdU/l1asbI6wvVPHGyjmSQVgSj aRWNuXhhbaT5W9o0rUlUlwPO/kEBtokMpGynrlgLTqdaxFzNM4ia0wHBUbb7tDOO whiTdZ43c0uMQg8Stp+30XmcuIhJwjv9ZIJVpdMOSfmM3URd67PpyUHQJMIHZnJs LqbIG4QJUCAVlxVF8zy6ETtKXRsRpJeqvclrDSL0sRFaP18c2iZ3ykDhzq3AFYtt ffZpAvlO5DRTc+SlKtIs/P9YwZGiPhaHaT5Kfis5XPRuOCc83rk= =0/YR -----END PGP SIGNATURE----- --=-=-=--