On Mon, Sep 25, 2023 at 10:18:55AM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 25 Sept 2023 at 09:13, Tom Rini <tr...@konsulko.com> wrote: > > > > On Mon, Sep 25, 2023 at 08:35:44AM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Mon, 25 Sept 2023 at 08:08, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > On Sun, Sep 24, 2023 at 08:39:49PM -0600, Simon Glass wrote: > > > > > > > > > This series removes two more header files from the common.h header. It > > > > > also tidies up code style for time.h functions, to simplify similar > > > > > maintenance in future. > > > > > > > > > > > > > > > Simon Glass (3): > > > > > Fix code style for time functions > > > > > common: Drop time.h from common header > > > > > common: Drop linux/string.h from common header > > > > [snip] > > > > > 1709 files changed, 1854 insertions(+), 34 deletions(-) > > > > > > > > This is where I start to get worried we're possibly taking the wrong > > > > approach to the problem. Perhaps it would be better to: > > > > 1. Start finding headers which include <common.h>, remove and fixup the > > > > breakage directly. > > > > 2. Move on to removing <common.h> from files, a directory at a time. > > > > > > > > > > The bigger question is whether you want common.h gone or not? > > > > Yes, I'd like it gone. > > > > > I shared my script with you so you can see what it is doing. But if a > > > board uses memcmp(), for example, then it needs linux/string.h > > > included. We can discuss why so many files use string functions, or > > > whether the script is not perfect, but ultimately that is the > > > question. > > > > > > Just these simple queries give you a sense of the scale: > > > > > > git grep -l strcat |wc > > > 45 45 1134 > > > git grep -l memcpy |wc > > > 834 834 21643 > > > > > > If you are suggesting that we add fewer includes for now, relying on > > > transitive includes in header files, then I don't think that is very > > > useful in the end. I had a series a few years back which removed > > > common.h entirely and it foundered on this same issue. > > > > The problem I see with the approach you're taking now is that it's quite > > hard to provide meaningful review. Looking at the time.h patch here, I > > assume you agree it would be better to just remove > > arch/arm/include/asm/arch-tegra/timer.h as it's just a single prototype > > already found in include/time.h. And the example where time.h was > > included inside a comment block is a tooling issue. But then looking at > > the printk.h patch I did just apply, I had to drop a few files because > > they introduced seemingly random changes to the resulting build on some > > platforms. > > That is very strange. But I think with the Kconfig thing we > established that the build size can vary slightly for all sorts of > reasons, nothing to do with actual functional changes, particularly > with LTO. So as things standard, a build-size change isn't a good > signal.
I think, LTO platforms aside, it's extremely important and correct. If you aren't intending a functional change, there shouldn't be a functional change. We need to be able to explain why a change is happening if we're "just" changing headers. > > And something like that is much easier to deal with and > > explain _why_ it's happening if we're removing the header file by file, > > rather than world-wide move-a-header. > > With the Kconfig series I was unable to explain the minor 4-8 byte > changes, despite quite a bit of effort, as you know. > > My suggestion is that we press ahead with the migration. If some > maintainers want to do clean-up afterwards, then that would be great. I'd rather go with what I said about removing common.h from files and fixing them up as needed, and make any new code not include <common.h> since it's long just been "add headers X/Y/Z" and no prototypes itself. And it's exceedingly hard to review patches like this series has. -- Tom
signature.asc
Description: PGP signature