David Lechner wrote: > So far, I have not found scoped_cond_guard() to be nice to work with. > We have been using it quite a bit in the IIO subsystem via the > iio_device_claim_direct_scoped() macro. > > The main thing I don't like is that scoped_cond_guard() uses a for loop > internally. In the IIO subsystem, we usually try to return as early as > possible, so often we are returning from all paths from withing this > hidden for loop. However, since it is a for loop, the compiler thinks > that it possible to exit the for loop and so we end up having to use > unreachable() after the end of the scope to avoid a compiler warning. > This is illustrated in the ad7380 patch in this series and there are 36 > more instance of unreachable() already introduced in the IIO subsystem > because of this. > > Also, scoped_cond_guard() is they only macro for conditional guards in > cleanup.h currently. This means that so far, patches adopting this are > generally converting something that wasn't scoped to be scoped. This > results in changing the indentation of a lot of lines of code, which is > just noise in the patches. > > To avoid these issues, the natural thing to do would be to have a > non-scoped version of the scoped_cond_guard() macro. There was was a > rejected attempt to do this in [1], where one of the complaints was: > > > > - rc = down_read_interruptible(&cxl_region_rwsem); > > > - if (rc) > > > - return rc; > > > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > > > Yeah, this is an example of how NOT to do things. > > > > If you can't make the syntax be something clean and sane like > > > > if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) > > return -EINTR; > > > > then this code should simply not be converted to guards AT ALL. > > [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > I couldn't find a way to make a cond_guard() macro that would work like > exactly as suggested (the problem is that you can't declare a variable > in the condition expression of an if statement in C). So I am proposing > a macro that reads basically the same as the above so it still reads > almost like normal C code even though it hides the if statement a bit. > > if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem) > return -EINTR; > > The "not" is baked into the macro because in most cases, failing to > obtain the lock is the abnormal condition and generally we want to have > the abnormal path be the indented one. I think you could also take the "cond" out of the name because that is implied by the fact it's an 'if'. So, calling this "if_not_guard ()", for the series, you can add: Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx> ...but it's ultimately up to Peter and Linus if they find this "if ()" rename acceptable. If it is I would suggest the style should be treat it as an "if ()" statement and add this to .clang-format: diff --git a/.clang-format b/.clang-format index 252820d9c80a..ae3511a69896 100644 --- a/.clang-format +++ b/.clang-format @@ -63,6 +63,8 @@ DerivePointerAlignment: false DisableFormat: false ExperimentalAutoDetectBinPacking: false FixNamespaceComments: false +IfMacros: + - 'if_not_guard' # Taken from: # git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \ Last note, while the iio conversion looks correct to me, I would feel more comfortable if there was a way to have the compiler catch that plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring iio_device_claim_direct_mode() as __must_check achieves that effect?
