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

[Indu Bhagat <indu.bhagat@oracle.com>]

Hi Mike,

On 10/27/22 11:07, Mike Frysinger wrote:
> 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.
> 

OK. I have made the change in V3 to avoid recursive make.

>> +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 ?
> 

Removed -std=gnu99.

>> +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 ?
> 

I have sanitized the bits around warning flags, and the code should be 
warning-free now. Also addressed the other comments above.

>> +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 ?
> 

I saw the commit d546b61084cec687e0063b2e0e169b4690341c23 and judged 
that this may be needed.

>> --- /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

OK, changed to the suggested style.

Thanks for reviewing. I have addressed the comments in V3.