Re: [PATCH 2/3] decrease visibility of local symbols

[Tim Rice]

Hey Georg,

This is another one where I think the general principle seems sound, but I'd 
like to have more context. What's the motivation for making this change?

A web search on the topic of why bother hiding internal symbols with static in 
C gave results that were more focused on *how* rather than *why*.

~ Tim


On Sun, Jan 26, 2025 at 09:03:41PM +0100, Georg Sauthoff wrote:
> i.e. symbols that are only used inside their translation unit and don't
> have a prototype declaration in a header
> ---
> src/crosstab.c           |  2 +-
> src/datamash.c           | 10 +++++-----
> src/decorate-functions.c | 18 +++++++++---------
> src/decorate.c           | 18 +++++++++---------
> src/field-ops.c          |  8 ++++----
> 5 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/src/crosstab.c b/src/crosstab.c
> index de6cab7..637dd89 100644
> --- a/src/crosstab.c
> +++ b/src/crosstab.c
> @@ -75,7 +75,7 @@ crosstab_datacell_comparator (const void* a, const void* b)
> }
>
>
> -struct crosstab_datacell*
> +static struct crosstab_datacell*
> new_datacell (const char* row, const char* col, const char* data)
> {
>   struct crosstab_datacell *dc = xmalloc (sizeof (struct crosstab_datacell));
> diff --git a/src/datamash.c b/src/datamash.c
> index 3b5e5eb..24b0c60 100644
> --- a/src/datamash.c
> +++ b/src/datamash.c
> @@ -157,7 +157,7 @@ static struct option const long_options[] =
>   {NULL, 0, NULL, 0},
> };
>
> -void
> +static void
> group_columns_find_named_columns ()
> {
>   for (size_t i = 0; i < dm->num_grps; ++i)
> @@ -176,7 +176,7 @@ group_columns_find_named_columns ()
>     }
> }
>
> -void
> +static void
> usage (int status)
> {
>   if (status != EXIT_SUCCESS)
> @@ -539,7 +539,7 @@ print_column_headers ()
>   print_line_separator ();
> }
>
> -void
> +static void
> field_op_find_named_columns ()
> {
>   for (size_t i=0; i<dm->num_ops; ++i)
> @@ -578,7 +578,7 @@ process_input_header (FILE *stream)
>   line_record_free (&lr);
> }
>
> -void
> +static void
> summarize_field_ops ()
> {
>   for (size_t i=0;i<dm->num_ops;++i)
> @@ -599,7 +599,7 @@ summarize_field_ops ()
>   print_line_separator ();
> }
>
> -void
> +static void
> reset_field_ops ()
> {
>   for (size_t i=0;i<dm->num_ops;++i)
> diff --git a/src/decorate-functions.c b/src/decorate-functions.c
> index f14f647..bd9d4b8 100644
> --- a/src/decorate-functions.c
> +++ b/src/decorate-functions.c
> @@ -30,14 +30,14 @@
> #include "decorate-functions.h"
>
>
> -bool
> +static bool
> decorate_as_is (const char* in)
> {
>   fprintf (stdout, "%s", in);
>   return true;
> }
>
> -bool
> +static bool
> decorate_strlen (const char* in)
> {
>   uintmax_t u = (uintmax_t)strlen (in);
> @@ -45,7 +45,7 @@ decorate_strlen (const char* in)
>   return true;
> }
>
> -_GL_ATTRIBUTE_CONST int
> +static _GL_ATTRIBUTE_CONST int
> roman_numeral_to_value (char c)
> {
>   switch (c)
> @@ -66,7 +66,7 @@ roman_numeral_to_value (char c)
>    Does not support alternative forms such as
>     XIIX,IIXX for 18,
>     IIC for 98.  */
> -bool
> +static bool
> decorate_roman_numerals (const char* in)
> {
>   intmax_t result = 0;
> @@ -107,7 +107,7 @@ decorate_roman_numerals (const char* in)
>   return true;
> }
>
> -bool
> +static bool
> decorate_ipv4_inet_addr (const char* in)
> {
>   struct in_addr adr;
> @@ -127,7 +127,7 @@ decorate_ipv4_inet_addr (const char* in)
> }
>
>
> -bool
> +static bool
> decorate_ipv4_dot_decimal (const char* in)
> {
>   struct in_addr adr;
> @@ -149,7 +149,7 @@ decorate_ipv4_dot_decimal (const char* in)
> }
>
>
> -bool
> +static bool
> decorate_ipv6 (const char* in)
> {
>   struct in6_addr adr;
> @@ -207,14 +207,14 @@ decorate_ipv6_ipv4 (const char* in, uint32_t mapping)
> }
>
>
> -bool
> +static bool
> decorate_ipv6_ipv4_mapped (const char* in)
> {
>   return decorate_ipv6_ipv4 (in, 0xFFFF);
> }
>
>
> -bool
> +static bool
> decorate_ipv6_ipv4_compat (const char* in)
> {
>   return decorate_ipv6_ipv4 (in, 0);
> diff --git a/src/decorate.c b/src/decorate.c
> index ac50efb..88b95db 100644
> --- a/src/decorate.c
> +++ b/src/decorate.c
> @@ -125,7 +125,7 @@ static struct option const longopts[] =
> };
>
>
> -void
> +static void
> add_sort_extra_args (char* s)
> {
>   if (sort_extra_args_used == sort_extra_args_allocated )
> @@ -135,7 +135,7 @@ add_sort_extra_args (char* s)
>   sort_extra_args[sort_extra_args_used++] = xstrdup (s);
> }
>
> -void
> +static void
> usage (int status)
> {
>   if (status != EXIT_SUCCESS)
> @@ -540,7 +540,7 @@ undecorate_file (const char *infile, int num_fields)
>
>
> /* this code was copied from src/sort.c */
> -struct keyfield*
> +static struct keyfield*
> parse_sort_key_arg (const char* optarg, char const** endpos)
> {
>   char const *s;
> @@ -646,7 +646,7 @@ parse_builtin_conversion_spec (const char* optarg, const 
> char *s,
>     badfieldspec (optarg, N_("invalid built-in conversion option"));
> }
>
> -void check_allowed_key_flags (const struct keyfield* key)
> +static void check_allowed_key_flags (const struct keyfield* key)
> {
>   /* key->reverse is the only ordering flag allowed */
>   if (key->skipsblanks || key->skipeblanks
> @@ -658,7 +658,7 @@ void check_allowed_key_flags (const struct keyfield* key)
>                              "not be combined with a conversion function"));
> }
>
> -void parse_key_arg (const char* optarg)
> +static void parse_key_arg (const char* optarg)
> {
>   char const *s;
>   struct keyfield *key = parse_sort_key_arg (optarg, &s);
> @@ -690,7 +690,7 @@ void parse_key_arg (const char* optarg)
>     }
> }
>
> -void
> +static void
> insert_decorate_key (struct keyfield *key_arg)
> {
>   struct keyfield **p;
> @@ -706,7 +706,7 @@ insert_decorate_key (struct keyfield *key_arg)
> }
>
>
> -int
> +static int
> adjust_key_fields ()
> {
>   struct keyfield *key = keylist;
> @@ -753,7 +753,7 @@ adjust_key_fields ()
>   return cnt;
> }
>
> -char**
> +static char**
> build_sort_process_args ()
> {
>   int argc = 3 ; /* one 'sort' program name (argv[0]), one extra arg on NetBSD,
> @@ -797,7 +797,7 @@ build_sort_process_args ()
>   return argv;
> }
>
> -void
> +static void
> free_sort_args (char **args)
> {
>   for (char **arg = args; *arg; ++arg) {
> diff --git a/src/field-ops.c b/src/field-ops.c
> index f38fac7..6382218 100644
> --- a/src/field-ops.c
> +++ b/src/field-ops.c
> @@ -239,7 +239,7 @@ field_op_to_hex ( struct fieldop* op, const char *buffer, 
> const size_t inlen )
> }
>
> /* Add a string to the strings vector, allocating memory as needed */
> -void
> +static void
> field_op_add_string (struct fieldop *op, const char* str, size_t slen)
> {
>   if (op->str_buf_used + slen+1 >= op->str_buf_alloc)
> @@ -256,7 +256,7 @@ field_op_add_string (struct fieldop *op, const char* str, 
> size_t slen)
>
> /* Replace the current string in the string buffer.
>    This function assumes only one string is stored in the buffer. */
> -void
> +static void
> field_op_replace_string (struct fieldop *op, const char* str, size_t slen)
> {
>   if (slen+1 >= op->str_buf_alloc)
> @@ -677,7 +677,7 @@ unique_value ( struct fieldop *op, bool case_sensitive )
> }
>
> /* Returns the number of unique string values in the given field operation */
> -size_t
> +static size_t
> count_unique_values ( struct fieldop *op, bool case_sensitive )
> {
>   const char *last_str, **cur_str;
> @@ -709,7 +709,7 @@ count_unique_values ( struct fieldop *op, bool 
> case_sensitive )
>
> /* Returns a nul-terimated string, composed of all the values
>    of the input strings. The return string must be free'd. */
> -void
> +static void
> collapse_value ( struct fieldop *op )
> {
>   /* Copy the string buffer as-is */
> --
> 2.47.1