Hello! See my review notes below.
On Tuesday 15 December 2015 12:47:40 Kamil Rytarowski wrote:
> There is no way to check CPU type in a portable way across ABIs.
> Checking for sizeof(void*) is reasonable since most platforms will
> report correct values. One exception is x32, but since it's halfbaked
> never finished and almost not needed any more - we can ignore it.
Note that x32 is used in Linux world by e.g Debian or Gentoo. I do not
like to filter it just because of NetBSD.
> This change fixes build on NetBSD.
> ---
> configure.ac | 3 +++
> src/pulsecore/sample-util.h | 6 ++++--
> src/tests/mult-s16-test.c | 5 +++--
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index b9cd3d1..7735081 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -463,6 +463,9 @@ AC_TYPE_OFF_T
> AC_TYPE_UID_T
> AC_CHECK_DECLS(environ)
>
> +# Used to deduct CPU word size
> +AC_CHECK_SIZEOF(void*)
> +
> # SIGXCPU
> AX_CHECK_DEFINE([signal.h], [SIGXCPU], [HAVE_SIGXCPU=1], [HAVE_SIGXCPU=0])
> AS_IF([test "x$HAVE_SIGXCPU" = "x1"], AC_DEFINE([HAVE_SIGXCPU], 1, [Have SIGXCPU?]))
> diff --git a/src/pulsecore/sample-util.h b/src/pulsecore/sample-util.h
> index c817bc9..bf550d1 100644
> --- a/src/pulsecore/sample-util.h
> +++ b/src/pulsecore/sample-util.h
> @@ -55,10 +55,10 @@ void pa_deinterleave(const void *src, void *dst[], unsigned channels, size_t ss,
> void pa_sample_clamp(pa_sample_format_t format, void *dst, size_t dstr, const void *src, size_t sstr, unsigned n);
>
> static inline int32_t pa_mult_s16_volume(int16_t v, int32_t cv) {
> -#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
> +#if (SIZEOF_VOIDP * CHAR_BIT) == 64
> /* Multiply with 64 bit integers on 64 bit platforms */
> return (v * (int64_t) cv) >> 16;
> -#else
> +#elif (SIZEOF_VOIDP * CHAR_BIT) == 32
> /* Multiplying the 32 bit volume factor with the
> * 16 bit sample might result in an 48 bit value. We
> * want to do without 64 bit integers and hence do
> @@ -68,6 +68,8 @@ static inline int32_t pa_mult_s16_volume(int16_t v, int32_t cv) {
> int32_t hi = cv >> 16;
> int32_t lo = cv & 0xFFFF;
> return ((v * lo) >> 16) + (v * hi);
> +#else
> +#error Do not know if this is 32- or 64-bit code.
> #endif
> }
>
> diff --git a/src/tests/mult-s16-test.c b/src/tests/mult-s16-test.c
> index d2a351c..ac5a43f 100644
> --- a/src/tests/mult-s16-test.c
> +++ b/src/tests/mult-s16-test.c
> @@ -23,6 +23,7 @@
> #include <unistd.h>
> #include <stdlib.h>
> #include <math.h>
> +#include <limits.h>
>
> #include <pulse/rtclock.h>
> #include <pulsecore/random.h>
> @@ -93,9 +94,9 @@ int main(int argc, char *argv[]) {
> if (!getenv("MAKE_CHECK"))
> pa_log_set_level(PA_LOG_DEBUG);
>
> -#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
> +#if (SIZEOF_VOIDP * CHAR_BIT) == 64
> pa_log_debug("This seems to be 64-bit code.");
> -#elif __WORDSIZE == 32
> +#elif (SIZEOF_VOIDP * CHAR_BIT) == 32
> pa_log_debug("This seems to be 32-bit code.");
> #else
> pa_log_debug("Don't know if this is 32- or 64-bit code.");
And second note about patch:
Code which *needs* to know if machine is 32 or 64 bit is:
1) either buggy, wrong and incorrectly written
2) or architecture, platform or system specific
If this code is 1) then it should be fixed instead adding new hooks and
workaround about it. And if it is 2) then you should use correct
architecture/platform/system detection routine. That is *never* platform
independent, trying to do that is wrong way.
--
Pali Rohár
pali.rohar at gmail.com
