On Thu, Mar 6, 2025 at 4:38 AM Petr Mladek pmladek@suse.com wrote:
On Wed 2025-03-05 10:57:47, Tamir Duberstein wrote:
On Wed, Mar 5, 2025 at 10:55 AM Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
On Wed, Mar 05, 2025 at 10:25:51AM -0500, Tamir Duberstein wrote:
On Wed, Mar 5, 2025 at 10:01 AM Petr Mladek pmladek@suse.com wrote:
On Fri 2025-02-14 11:20:01, Tamir Duberstein wrote:
...
#include <kunit/test.h> -#include <linux/bitops.h> -#include <linux/kernel.h> #include <linux/module.h> -#include <linux/overflow.h> -#include <linux/printk.h> #include <linux/prandom.h> #include <linux/slab.h> -#include <linux/string.h> +#include <linux/sprintf.h>
#define BUF_SIZE 1024
It would make more sense to do this clean up in the 3rd patch where some code was replaced by the kunit macros.
I would personally prefer to keep the explicit includes when the related API is still used. It helps to optimize nested includes in the header files which helps to speedup build. AFAIK, there are people working in this optimization and they might need to revert this change.
Yeah, I don't feel strongly. I'll just restore all the includes.
It will be blind approach. Please, try to look at them closely and include what you use (IWYU principle). I don't think anybody uses kernel.h here, for example.
I think I'm getting conflicting instructions here. IWYU is indeed what I did: bitops, kernel, overflow, printk are all unused;
I believe that the headers were added for a reason. And this patchset keeps most of the code. This is why the change look suspicious. And I see in the patched lib/tests/scanf_kunit.c:
- hweight32(), BITS_PER_TYPE(), BITS_PER_LONG which looks like bitops stuff
- is_signed_type(), type_min(), type_max() from overflow.h
So, I would keep bitops.h and overflow.h.
Thanks for checking.
The printk() calls were obviously removed in the 3rd patch so printk.h include should be removed there.
I do not see any obvious reason for kernel.h, so I would remove it in a separate patch.
string is used only for sprintf, so I made that replacement.
Makes sense. But please do this in a separate patch with this explanation. It might be done together with the kernel.h removal.
Will do.
Thanks again. Tamir