This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] PowerPC - Add a faster way to read the Time Base register
> Roland, I noticed in other thread that you think that all public static
> inline should have __attribute__ ((__unused__)). Do you think these
> static inlines should also have this attribute even if I haven't noticed
> the related warning in my tests?
Don't worry about it for now. It turns out we already have tons of
instances without it. If/when we decide that we really want it, we'll add
it everywhere en masse and have some sys/cdefs.h macro for it probably.
For now, it's easier not to open the can of worms of compiler version
compatibility and so forth.
> +@Theglibc{} provided platform header should coordinate with GCC such
This means to read as "The ... library-provided", but hyphenation like that
is not very wise with the macro. So instead use a construction like, "The
platform header provided by @theglibc{} should..."
> +that compiler built-in versions of the functions and macros are
> +preferred if available. This allows user programs to need only ever
> +include @file{sys/platform/@var{arch}.h}, keeping the same name of types,
> +macros, and functions for convenience and portability.
"This means that user programs will only ever need to include..."
"...same names of types, ..."
> +@item
> +Each included symbol must have the prefix @file{__@var{arch}_} e.g.
> +@file{__ppc_get_timebase}.
comma or semicolon before e.g.
> +The easiest way to ship a header is to add it to the sysdep_headers
@code{sysdep_headers}
> +variable, for example, the combination of Linux-specific headers on
"For example" should start a new sentence here.
> +PowerPC could be provided like this:
> +
> +@smallexample
> +sysdeps_header += sys/platform/ppc.h
s/sysdeps_header/&s/
> +Then ensure that you have checked in a @file{sys/platform/ppc.h}
> +header underneath your target platform sysdeps directory e.g.,
> +@file{sysdeps/powerpc/sys/platform/ppc.h}.
Say "added" rather than "checked in".
> +
> @node Porting
> @appendixsec Porting @theglibc{}
>
> diff --git a/manual/platform.texi b/manual/platform.texi
> new file mode 100644
> index 0000000..9aceb26
> --- /dev/null
> +++ b/manual/platform.texi
> @@ -0,0 +1,29 @@
> +@node Platform, Contributors, Maintenance, Top
> +@c %MENU% Describe all platform specific facilities provided
> +@appendix Summary of platform specific facilities
"platform-specific" (other instances follow)
> +@menu
> +* Power Architecture Facilities:: Summary of Power Architecture
> + specific facilities
> +@end menu
In the example above and in GNU configuration it's called "PowerPC",
not "Power". Be consistent.
> +@item typedef unsigned long long int __ppc_timebase
> +Represents the value read from the timebase register.
There is no point in having a typedef if the manual says what the
underlying type is. Just say something like, "This is an unsigned integer
type that represents..." Use @deftp here.
> +@item __ppc_timebase __ppc_get_timebase (void)
> +Read the timebase register without the need of a system call.
Use @deftypefun here.
> +#ifndef _SYS_PLATFORM_PPC_H
> +
> +#define _SYS_PLATFORM_PPC_H 1
No blank line between these two.
> +typedef unsigned long long int __ppc_timebase;
> +
> +/* Read the Time Base Register
> + The Time Base Register is a 64-bit register that stores a monotonically
> + incremented value updated at a system dependent frequency that may be
> + different of processor frequency.
> + More information in Power ISA 2.06b - Book II - Section 5.2 */
"system-dependent". "different from the".
> +#ifdef __powerpc64__
> +static inline __ppc_timebase
> +__ppc_get_timebase (void)
> +{
Put the #ifdef inside the function body.
> +/* Copyright (C) 2012 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
Always make the top line a descriptive comment for the file.
> + Contributed by Tulio Magno Quites Machado <tuliom@linux.vnet.ibm.com>, 2012.
Recent consensus is not to add any new "Contributed by" lines.
You'll get credit in the ChangeLog and NEWS files.
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, write to the Free
> + Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + 02111-1307 USA. */
This boilerplate is old. Use an example from a current public header file.
This file probably needs the "special exception" text.
> +/* Test if __ppc_get_timebase() is compatible with the current processor and if
> + it's changing between reads.
> + In case of a read failure it may indicate a future Power ISA or a binutils
> + change. */
New tests should get a copyright header too.
> +do_test(void)
Missing space.
> + if ((t1 != t2) && (t1 != t3) && (t2 != t3))
No need for these extra parens.
Thanks,
Roland