Re: [PATCH,V2 05/15] libsframe: add the SFrame library

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 3098 bytes --]

On 17 Oct 2022 15:16, Indu Bhagat via Binutils wrote:
> --- /dev/null
> +++ b/libsframe/Makefile.am
>
> +SUBDIRS = testsuite

since this is new code, can you avoid recurisve makes ?  rename the subdir
Makefile.am to local.mk and include them in this Makefile.am instead.

> +BASEDIR = $(srcdir)/..

you don't actually use this anywhere

> +AM_CPPFLAGS = -D_GNU_SOURCE -I$(srcdir) -I$(srcdir)/../include -I$(srcdir)/../libctf

you called AC_USE_SYSTEM_EXTENSIONS, so why do you need to use -D_GNU_SOURCE ?

> +AM_CFLAGS = -std=gnu99 @ac_libsframe_warn_cflags@ @warn@ @c_warn@ @WARN_PEDANTIC@ @WERROR@

most other projects have already moved to C11.  why do you have to use such
an ancient version ?

> +libsframe_la_CPPFLAGS = -D_GNU_SOURCE -I$(srcdir) -I$(srcdir)/../include \
> +			-I$(srcdir)/../libctf

you should be using $(AM_CPPFLAGS) instead of manually duplicating the value

> --- /dev/null
> +++ b/libsframe/configure.ac
>
> +AC_CONFIG_MACRO_DIR(..)
> +AC_CONFIG_MACRO_DIR(../config)
> +AC_CONFIG_MACRO_DIR(../bfd)

you're setting ACLOCAL_AMFLAGS already.  pretty sure you don't need these.

> +MISSING=`cd $ac_aux_dir && ${PWDCMD-pwd}`/missing
> +AC_CHECK_PROGS([ACLOCAL], [aclocal], [$MISSING aclocal])
> +AC_CHECK_PROGS([AUTOCONF], [autoconf], [$MISSING autoconf])
> +AC_CHECK_PROGS([AUTOHEADER], [autoheader], [$MISSING autoheader])

why do you need this logic ?  isn't this covered by AM_MAINTAINER_MODE ?

> +# Figure out what compiler warnings we can enable.
> +# See config/warnings.m4 for details.

use `dnl` for comments, not `#`

> +ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wno-narrowing -Wwrite-strings \
> +			  -Wmissing-format-attribute], [warn])
> +ACX_PROG_CC_WARNING_OPTS([-Wstrict-prototypes -Wmissing-prototypes \
> +			  -Wold-style-definition], [c_warn])
> +ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([-Wno-long-long])
> +
> +# Only enable with --enable-werror-always until existing warnings are
> +# corrected.
> +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual])

this project is new.  why aren't the warnings fixed to begin with ?

> +DEJAGNU_CHECK_VERSION
> +AM_CONDITIONAL([HAVE_COMPAT_DEJAGNU], [test "x$ac_cv_dejagnu_compat" = "xyes"])
> +
> +COMPAT_DEJAGNU=$ac_cv_dejagnu_compat
> +AC_SUBST(COMPAT_DEJAGNU)
> +
> +AM_MAINTAINER_MODE
> +AM_INSTALL_LIBBFD
> +ACX_PROG_CC_WARNING_OPTS([-Wall], [ac_libsframe_warn_cflags])

you already probed -Wall above.  do you really need to do it again ?

> +AC_FUNC_MMAP
> +AC_CHECK_HEADERS(byteswap.h endian.h)
> +
> +dnl Check for bswap_{16,32,64}
> +AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64], [], [], [[#include <byteswap.h>]])
> +AC_CHECK_DECLS([asprintf, vasprintf, stpcpy])

you aren't using most of this.  are you just copying & pasting from another
tree ?

> +GNU_MAKE_JOBSERVER

do you actually need this ?

> --- /dev/null
> +++ b/libsframe/testsuite/libsframe.decode/Makefile.am
> @@ -0,0 +1,17 @@
> +if HAVE_COMPAT_DEJAGNU
> +  check_PROGRAMS = be-flipping frecnt-1 frecnt-2
> +else
> +  check_PROGRAMS =
> +endif

this style complicates things.  use:

check_PROGRAMS =
if HAVE_COMPAT_DEJAGNU
check_PROGRAMS += ...
endif
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]