Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 28 Oct 2013 04:01:29 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Ian Lepore <ian@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r257200 - in head/sys/arm: allwinner allwinner/a20 arm at91 broadcom/bcm2835 econa freescale/imx include lpc mv rockchip samsung/exynos tegra ti ti/am335x ti/omap4 ti/twl versatile xili...
Message-ID:  <20131028030951.V929@besplex.bde.org>
In-Reply-To: <201310270134.r9R1YBbI037760@svn.freebsd.org>
References:  <201310270134.r9R1YBbI037760@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 27 Oct 2013, Ian Lepore wrote:

> Log:
>  Remove #include <machine/frame.h> from all the arm code that doesn't
>  really need it.  That would be almost everywhere it was included.  Add
>  it in a couple files that really do need it and were previously getting
>  it by accident via another header.

Oops, I complained too soon about style bugs attached to these includes
in another reply.  Removing the includes also removes most of the
collateral style bugs.

How do you find the files that _really_ do need it?  Nested pollution
makes this difficult.  tools/kerninclude barely works.  When I tried
to reduce nested pollution, I used a predecessor of tools/kerninclude
that was more usable because it was more localized, but it got other
things wrong.

> Modified: head/sys/arm/include/undefined.h
> ==============================================================================
> --- head/sys/arm/include/undefined.h	Sun Oct 27 00:51:46 2013	(r257199)
> +++ head/sys/arm/include/undefined.h	Sun Oct 27 01:34:10 2013	(r257200)
> @@ -52,7 +52,9 @@
>
> #include <sys/queue.h>
>
> -typedef int (*undef_handler_t) (unsigned int, unsigned int, trapframe_t *, int);
> +struct trapframe;
> +
> +typedef int (*undef_handler_t) (unsigned int, unsigned int, struct trapframe *, int);
>
> #define FP_COPROC	1
> #define FP_COPROC2	2

Other uses of the trapframe_t style bug reqiore more editing.

This adds a style bug (extra blank line which separates the forward
declaration from its one use).

Old style bugs remaining in the main changed line include:
- not using u_int (perhaps it should be foo_t)
- gnu style parentheses
- long line

I don't like extensive use of typedefs to obfuscate function pointers,
even without the style bug of declaring a typedef for the pointer and
not a typedef for the function.  We use typedefs for function pointers
because the syntax for a function pointer is complicated, not primarily
to hide the type.  Here the pointer is used only a few times so repeating
the declaration would be clearer.  However, if the typedef were for the
function then it could be used more.  Now, most of the few uses are for
a public function that seems to be never called:
- install_coproc_handler() is declared in this header and its declaration
   uses the typedef
- install_coproc_handler() is defined in arm/undefined.c and its definition
   uses the typedef
- install_coproc_handler() seems to be never called.  The installation
   function that is actually called is install_coproc_handler_static().
   This was apparently originally a static internal for
   install_coproc_handler().  It is still named with 'static' and is still
   used internally but is now public and used externally.
- a critical struct declaration in this header also uses the typedef.
   install_coproc_handler_static() takes a pointer to this struct as an
   arg so its callers don't need the typedef.  Since the typedef is for
   a pointer and not a function, it cannot be used in forward declarations
   of handlers.  Even if the typedef were for a function, C syntax wouldn't
   allow using it in definitions of handlers.
So the typedef ends up being used once in used code and twice in unused code.

Old style bugs visible in unchanged lines:
- space instead of tab after #define.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131028030951.V929>