From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.gentoo.org (smtp.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) by sourceware.org (Postfix) with ESMTP id 8F1733857434 for ; Thu, 27 Oct 2022 19:21:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8F1733857434 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gentoo.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gentoo.org Received: by smtp.gentoo.org (Postfix, from userid 559) id 0AB0B340E96; Thu, 27 Oct 2022 19:21:49 +0000 (UTC) Date: Thu, 27 Oct 2022 23:52:01 +0545 From: Mike Frysinger To: Indu Bhagat Cc: binutils@sourceware.org Subject: Re: [PATCH,V2 05/15] libsframe: add the SFrame library Message-ID: Mail-Followup-To: Indu Bhagat , binutils@sourceware.org References: <20221017221612.495324-1-indu.bhagat@oracle.com> <20221017221612.495324-6-indu.bhagat@oracle.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bNYQ6Qk9jRBo/euK" Content-Disposition: inline In-Reply-To: <20221017221612.495324-6-indu.bhagat@oracle.com> X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --bNYQ6Qk9jRBo/euK Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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 ]]) > +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 --bNYQ6Qk9jRBo/euK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEuQK1JxMl+JKsJRrUQWM7n+g39YEFAmNayMUACgkQQWM7n+g3 9YHSHQ//SDINZ513/Kqt/Hd1CYlYa1iGQ6n8k82mJF63hHYdAKhlHuNqi+dV4PiN bCqtTq0OVvgs5awo0g9chLM2sFwlhMZGzJff9GW2NE70kThf8XxkCt9i7Bs2+UeY EzyvuVeGK60QMIkSqa53gejdHiMG+ACuY95aEkh7G3B5+C4e/sVk00EDsPZmCv6R xHPDlEnR19fwaSKpw+xtLfiOa7x1hSIs8rWnEbnzLM3D+LwYxGaIZs/2mzEkeykX psfViIMD37KD1XaC1c7nT48CgFnkqFH0KDgoigza1DylYaAcyW0koxFTDS4cC7AP 1EpCXPWQNE1HJNbrEKQljlw/dKOu9OPhV5eSeF1JvHVro5kWns9PmEFPeymWNqvL w85pbmgXX/Mq4KwwpFjXRgrvaFKueMKfMjAzyfyFqKuE2VBlj4wSXotTqhG4AM2E PVU42Dx6RN5RtPIHtJYWFWQ4PYRO4YOtS+9Vev7wj1OroFGK/YDYdiuZhu/YMhg3 fMfJ9zAuJ3UW6fXxTexi72uS0Qu2QtBMawxiC0KmuRExdBpVNexIAQEOv48lq4Db DEFWbuOmHdbK5TcYhInOUEe+mQ/2vwuavlrMOjF45/Q0cNzXLE1vrXzm0PIVHBqC YvrLdgKMEhFIfb4NHU+oqxxo4DbWiJaF9Rj71naUOS93kjlok70= =oMHo -----END PGP SIGNATURE----- --bNYQ6Qk9jRBo/euK--