The 4 split patches looked good to me. About the init value of enctype in tests/varlocs.c, yes, clang does not seem to know that error(EXIT_FAILURE, ...) won't return.
Yes, the labs should be llabs to work on 32-bit systems. About removing the NULL tests of parameters declared with NN, I don't mind removing them. I was extra careful not to remove any check in case some code could call those functions with NULL parameter and without the NN declaration. Please let me know if you need me to prepare another patch to remove those unnecessary NULL tests or replace abs() with llabs(). Thanks. On Mon, Sep 7, 2015 at 3:32 PM, Mark Wielaard <[email protected]> wrote: > Hi, > > I split this out in individual patches. Some are clearly good to get > cleaned up and get in immediately. But some might need some discussion > first. The four I think are fine and would like to commit are attached. > > On Fri, 2015-09-04 at 12:04 -0700, Chih-Hung Hsieh wrote: > > * Replace K&R function definition with prototypes to match their > > declarations. > > Clang gives errors of: promoted type 'int' of K&R function parameter > > is not compatible with the parameter type > > This is OK. I would like to get rid of the K&R function definitions in > general. They can hide some issues (see also below). We should probably > use gcc -Wold-style-definition to find them all. > > I am not really sure why just these few get flagged. Most cases seem to > be flagged because there are no explicit prototypes. In the backends > case the _init functions are properly called through the ebl_bhinit_t in > openbackend. There are no explicit prototypes for these functions > though. > > In the libasm FCT and UFCT case the various addintXX.c files include the > asm_addint8.c file to generate the various variants, the files don't > include any definitions. > > In the case of asm_begin, dwarf_next_cfi, __libdw_intern_next_unit, > __libdw_findcu I that the warning is because they all have a 'bool' > argument that might be promoted differently in pre-ansi code? > > I don't understand why there is a complaint about > ebl_openbackend_machine () and ebl_check_st_other_bits (). Those do look > fine to me with a declaration from libebl.h which is included. The issue > might again be that the last arguments might be promoted differently. > > If we want to get rid of the K&R function definitions then lets start > with these. I reformatted them a little so they confirm with the GNU > coding standards we use. > > > * Add const declaration to locs, which was passed a const. > > Clang gives errors of: passing 'const Elf_Data *' to parameter of > > type 'Elf_Data *' discards qualifiers > > Good. This was also caused by a K&R function definition. I changed it > also to a new style definition and gcc warns about this too. > > > * Avoid clang errors of: comparison of nonnull parameter ... equal to a > null pointer is false > > on first encounter [-Werror,-Wtautological-pointer-compare] > > The parameter was declared as non-null. > > If they are marked as nonnull arguments then I think we should just > remove the NULL checks. Casting away to get rid of the warning seems the > wrong approach. > > > * Replace abs with labs for int64 values. > > Nice catch. But labs is for longs, which might on some arches be only 32 > bits. Should we be using llabs? > > > * Remove unused static variables. > > This is fine. They are clearly unused. > BTW this is gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28901 > > > * Init local variable before use, where static analysis failed. > > OK, because it is just test code. But in general it seems bad to > unnecessary initialize variables. > > Thanks, > > Mark >
