Re: [PATCH 6/6] Use file stream or format variants of stdio print functions
Hi Michael,
On Thu, Oct 10, 2024 at 10:27:31AM +, Michael Pratt wrote:
> In many source files, putc() and fputc() and putchar()
> are used interchangeably, and puts() and fputs() and printf()
> are used interchangeably.
>
> Reducing the usage to 2 of the 3 both reduces binary size
> of the final product, improves readability and searchability
> of the code, especially since grepping for puts() can clash with
> words like "inputs" and "outputs", and that putc() can be a macro
> while fputc() is the safer function.
Aha. I should have read the whole series first before asking questions
on some of the previous commits. So this is why you (already) tried to
replace putchar_unlocked with fputc.
If we are trying to be more consistent then I think I would prefer we
use putchar, puts and printf, instead of adding stdout to all these
calls.
Cheers,
Mark
> While at it, adjust spacing syntax and punctuation for consistency.
>
> Signed-off-by: Michael Pratt
> ---
> libcpu/i386_disasm.c | 2 +-
> src/addr2line.c | 8 +--
> src/ar.c | 2 +-
> src/elfclassify.c | 2 +-
> src/elflint.c | 2 +-
> src/nm.c | 2 +-
> src/objdump.c | 4 +-
> src/readelf.c | 108 +-
> src/unstrip.c | 2 +-
> tests/addrcfi.c | 2 +-
> tests/addrscopes.c| 2 +-
> tests/all-dwarf-ranges.c | 4 +-
> tests/arextract.c | 4 +-
> tests/asm-tst1.c | 6 +-
> tests/asm-tst2.c | 6 +-
> tests/asm-tst3.c | 8 +--
> tests/asm-tst4.c | 2 +-
> tests/asm-tst5.c | 2 +-
> tests/asm-tst6.c | 2 +-
> tests/asm-tst7.c | 6 +-
> tests/asm-tst8.c | 6 +-
> tests/asm-tst9.c | 6 +-
> tests/buildid.c | 4 +-
> tests/debugaltlink.c | 4 +-
> tests/dwarf-getmacros.c | 6 +-
> tests/dwarf-getstring.c | 2 +-
> tests/dwarf-ranges.c | 2 +-
> tests/dwarfcfi.c | 2 +-
> tests/dwflmodtest.c | 6 +-
> tests/funcretval.c| 6 +-
> tests/funcscopes.c| 2 +-
> tests/get-aranges.c | 6 +-
> tests/get-files-define-file.c | 2 +-
> tests/get-files.c | 2 +-
> tests/get-pubnames.c | 4 +-
> tests/line2addr.c | 4 +-
> tests/next-files.c| 2 +-
> tests/saridx.c| 2 +-
> tests/scnnames.c | 2 +-
> tests/sectiondump.c | 4 +-
> tests/show-die-info.c | 62 +--
> tests/showptable.c| 2 +-
> tests/test-nlist.c| 2 +-
> 43 files changed, 159 insertions(+), 157 deletions(-)
>
> diff --git a/libcpu/i386_disasm.c b/libcpu/i386_disasm.c
> index dec62bfa..7f199b49 100644
> --- a/libcpu/i386_disasm.c
> +++ b/libcpu/i386_disasm.c
> @@ -565,7 +565,7 @@ i386_disasm (Ebl *ebl __attribute__((unused)),
> #endif
> default:
> /* Cannot happen. */
> - puts ("unknown prefix");
> + fputs ("unknown prefix\n", stdout);
> abort ();
> }
> data = begin + 1;
> diff --git a/src/addr2line.c b/src/addr2line.c
> index d87e5b45..9ced8a62 100644
> --- a/src/addr2line.c
> +++ b/src/addr2line.c
> @@ -729,10 +729,10 @@ handle_address (const char *string, Dwfl *dwfl)
> show_int (&dwarf_lineisa, info, "isa");
> show_int (&dwarf_linediscriminator, info, "discriminator");
> }
> - putchar ('\n');
> + fputc ('\n', stdout);
> }
>else
> -puts ("??:0");
> +fputs ("??:0\n", stdout);
>
>if (show_inlines)
> {
> @@ -811,10 +811,10 @@ handle_address (const char *string, Dwfl *dwfl)
> if (src != NULL)
> {
> print_src (src, lineno, linecol, &cu);
> - putchar ('\n');
> + fputc ('\n', stdout);
> }
> else
> - puts ("??:0");
> + fputs ("??:0\n", stdout);
> }
> }
> }
> diff --git a/src/ar.c b/src/ar.c
> index 9ace28b9..ad48c395 100644
> --- a/src/ar.c
> +++ b/src/ar.c
> @@ -579,7 +579,7 @@ do_oper_extract (int oper, const char *arfname, char
> **argv, int argc,
> if (oper == oper_list)
> {
> if (!verbose)
> - puts (arhdr->ar_name);
> + printf ("%s\n", arhdr->ar_name);
>
> goto next;
> }
> diff --git a/src/elfclassify.c b/src/elfclassify.c
> index 25fe9a65..8c18b5d1 100644
> --- a/src/elfclassify.c
> +++ b/src/elfclassify.c
> @@ -820,7 +820,7 @@ process_current_path (int *status)
> {
> case do_print:
>if (ch
Re: [PATCH 2/6] libcpu: Include config.h before standard headers in lexer source
Hi Michael, On Thu, Oct 10, 2024 at 10:27:02AM +, Michael Pratt wrote: > As part of the processing of flex, definitions and headers > are added to output source before any literal text or generated code. > > This causes standard headers to come before config.h > unless config.h is included in a %top block instead > as specified in the flex manual, section 5.1 "Format of the Definitions". > > The %top block is non-POSIX, so using it reinforces > the requirement of "flex" over a standardized "lex" even more. > > * libcpu/i386_lex.l (%top): add flex %top block > and move config.h header inclusion to it. configure already checks we have flex, so this is fine. Pushed, Mark
Re: [PATCH 1/6] lib: Add missing config.h include to next_prime.c
Hi Michael, On Thu, Oct 10, 2024 at 10:26:54AM +, Michael Pratt wrote: > This is the last remaining C source file as of this commit > without the standard conditional inclusion of config.h > as the very first header. > > * lib/next_prime.c: add missing config.h header. Looks good and is more consistent. Pushed. There isn't much code in this file. What kind of build issue are you seeing without this? Does config.h contain some construct that affects the build of this file? Thanks, Mark
Re: [PATCH 5/6] Remove usage of "unlocked" variant of stdio print functions
Hi Michael, On Thu, Oct 10, 2024 at 10:27:23AM +, Michael Pratt wrote: > These Linux Standard Base functions are not available > on all systems that are capable of building Linux and ELFs. > > The difference is negligible for simple printing to stdout. > > POSIX also states for the similar putc_unlocked(): > > These functions can safely be used in a multi-threaded program > if and only if they are called while the invoking thread owns > the (FILE *) object, as is the case after a successful call > to the flockfile() or ftrylockfile() functions. > > and > > These unlocked versions can be safely used > only within explicitly locked program regions, > using exported locking primitives. > > and this was never done. > > There is inconsistent use of fputc_unlocked() > mixed with putc_unlocked() and putchar_unlocked(), > so consistently use the safer fputc() instead. I agree the use of the _unlocked variants is inconsistent and it would be better to consistently use the standard versions. But shouldn't we simply replace putchar_unlocked with putchar instead of using fputc (c, stdout)? Cheers, Mark
Re: [PATCH 3/6] libdw: Let clean targets be unconditional
Hi Michael, On Thu, Oct 10, 2024 at 10:27:09AM +, Michael Pratt wrote: > The automake rule "maintainer-clean-generic" > is always available and never conditional, > so let the variable that uses it be define > non-conditionally. > > If one actually wants conditional cleaning > they should write a custom rule and set it > as a dependency of a "*clean-local" automake rule. > > There is no need to do conditional cleaning here, > so move the MAINTAINERCLEANFILES variable definition > to the end of the Makefile.am file as it is > in the rest of the project. > > * libdw/Makefile.am: move MAINTAINERCLEANFILES > variable to the end of the file > as a non-conditional definition. You are right, there is no reason to only define MAINTAINERCLEANFILES conditionally. Pushed. Thanks, Mark
Re: [PATCH 0/6] Various portability fixes and organization
Hi Aaron, The first 3 patches of the series are pretty simple and helpful. I would like to see them in the next release, do they look ok? Without the first 2, we actually experience build errors due to config.h not being included or being included too late. The 3rd one prevents an error with our patched automake, it's unknown whether upstream automake would accept it due to the increased strictness in how the variable is defined, but it's a very easy adjustment anyway. -- Thanks for your consideration, MCP
