Hi Pádraig,
A code review:
> +#ifndef _CPU_SUPPORTS_H
> +# define _CPU_SUPPORTS_H
Why does this double-inclusion guard not include the two #include statements?
gcc has a special optimization of includes, when the included .h file has
a double-inclusion guard that encompasses _all_ the other contents (except
comments).
> # define cpu_supports(feature) \
This seems to be the main macro of the .h file. Can you document it, please?
The comments should say:
- What are the valid feature strings? (pointer to the GCC documentation)
- What does this macro do on systems with glibc?
- What does this macro do on systems without glibc?
> + && __builtin_cpu_supports (feature))
The original code in lib/crc.c was using
0 < __builtin_cpu_supports (feature)
Can __builtin_cpu_supports return negative values or values > 1 ?
> + char const* hwcap = nullptr;
Yet another variant of writing 'char const *' ?
> +inline
That's overkill when all you need is 'static inline'. See
https://www.gnu.org/software/gnulib/manual/html_node/static-inline.html
> + static char const *GLIBC_TUNABLES;
By GNU convention, entirely uppercase identifiers are used only for macros
or enum elements. Plain static variables are lowercase, by convention.
These conventions help readability.
>+ char const *cap = GLIBC_TUNABLES;
>+ while ((cap = strstr (cap, glibc_hwcap)) && cap < sentinel)
This code is parsing the value of the GLIBC_TUNABLES environment
variable. It would be useful to include in comments a pointer to the
description of the expected syntax of that string or — if that does
not exist — a summary of that syntax.
> + while ((cap = strstr (cap, glibc_hwcap)) && cap < sentinel)
The '&& cap < sentinel' would be unnecessary if you were to previously
check that glibc_hwcap does not start with ':'.
Is it useful to accept as glibc_hwcap a string that starts with ':'?
gcc_feature_to_glibc_hwcap never returns such a string so far.
Bruno
