[PATCH 0/3] common: Remove a few more header files

[Simon Glass]

Hi Tom,

On Mon, 25 Sept 2023 at 09:13, Tom Rini <trini at 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 <trini at 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.

>  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.

Regards,
Simon