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

[Tom Rini]

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230925/306cc415/attachment.sig>