Date: Sun, 27 Oct 2013 10:46:18 -0600 From: Ian Lepore <ian@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r257199 - in head/sys/arm: allwinner arm broadcom/bcm2835 freescale/imx include lpc mv mv/orion rockchip sa11x0 samsung/exynos tegra ti ti/omap4/pandaboard versatile xilinx xscale/i8032... Message-ID: <1382892378.1170.170.camel@revolution.hippie.lan> In-Reply-To: <20131028015840.V929@besplex.bde.org> References: <201310270051.r9R0pl3j024025@svn.freebsd.org> <20131028015840.V929@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 2013-10-28 at 03:07 +1100, Bruce Evans wrote:
>
> > Modified: head/sys/arm/allwinner/a10_machdep.c
> > ==============================================================================
> > --- head/sys/arm/allwinner/a10_machdep.c Sat Oct 26 23:41:11 2013 (r257198)
> > +++ head/sys/arm/allwinner/a10_machdep.c Sun Oct 27 00:51:46 2013 (r257199)
> > @@ -45,7 +45,6 @@ __FBSDID("$FreeBSD$");
> > #include <machine/bus.h>
> > #include <machine/frame.h> /* For trapframe_t, used in <machine/machdep.h> */
> > #include <machine/machdep.h>
> > -#include <machine/pmap.h>
> >
> > #include <dev/fdt/fdt_common.h>
>
> Some other style bugs are visible in the patch.
>
> Saying what an include is for is a bogus "come from" comment. Such comments
> are usually rotten, and the above is no exception. trapframe_t is a
> style bug (an "opaque" type that must be visible to use it) that still
> exists (only arm has it, at least now). But arm/machine/machdep.h doesn't
> use it now. I don't know if machine/machdep.h needed to use it. But the
> optimization of passing full trap frames without copying them has been lost
> because it depended on undefined behaviour in C. Everything now passes
> pointers to trap frames instead. arm/machine/machdep.h spells the pointers
> correctly as "struct trapframe *" after forward-declaring an incomplete
> "struct trapframe", so it doesn't actually use trapframe_t. Perhaps the
> include is now included for some unclaimed reason.
>
>
The "come from" comment bogosity was mine, although the trapframe_t
unhappiness came first. I had at one time started flagged them as a
baby step towards cleaning them all up, then did nothing for months.
You may have noticed that I finally finished the job of eliminating
frame.h from most of the places that didn't actually need it in r257200.
While doing that, I noticed a whole lot of other includes in arm source
code that just aren't needed (things like cpu.h and intr.h in virtually
every file). So much of our arm code gets created by wholesale pasting
some other code and then hacking. That's not completely invalid, I do
it myself sometimes, but I generally start by commenting out most of the
includes, then uncomment until it compiles.
I didn't realize only arm had a trapframe_t. Now I'm inclined to just
seek and destroy the remnants of it.
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1382892378.1170.170.camel>