Re: [PATCH 3/3] fclose: pacify gcc -Wanalyzer-file-leak

[Bruno Haible]

Paul Eggert wrote:
> diff --git a/m4/fclose.m4 b/m4/fclose.m4
> index dc021dd9f3..74b7df0a77 100644
> --- a/m4/fclose.m4
> +++ b/m4/fclose.m4
> @@ -31,6 +31,10 @@ AC_DEFUN([gl_FUNC_FCLOSE],
>        *) REPLACE_FCLOSE=1 ;;
>      esac
>    fi
> +
> +  if test $REPLACE_FCLOSE = 1; then
> +    REPLACE_FOPEN=1
> +  fi
>  ])
>  
...
> diff --git a/modules/fclose b/modules/fclose
> index 703de9e686..9a949e48b8 100644
> --- a/modules/fclose
> +++ b/modules/fclose
> @@ -9,12 +9,18 @@ Depends-on:
...
>  configure.ac:
>  gl_FUNC_FCLOSE
> +if test $REPLACE_FCLOSE = 1; then
> +  REPLACE_FOPEN=1
> +  AC_LIBOBJ([fopen])
> +  gl_PREREQ_FOPEN
> +fi
>  gl_CONDITIONAL([GL_COND_OBJ_FCLOSE], [test $REPLACE_FCLOSE = 1])
>  gl_STDIO_MODULE_INDICATOR([fclose])

This patch is technically correct, but has maintainability problems: It
intermingles the actions of the modules 'fopen' and 'fclose'. In particular:
  * Setting the variable REPLACE_FOO=1 from elsewhere than m4/foo.m4 is
    not a good practice, because
      (1) It is likely to lead to bugs, depending on the order in which the
          various gl_FUNC_* macros are expanded.
      (2) It is likely to lead to a bug the next time module 'foo' is being
          modified.
  * Invoking AC_LIBOBJ([foo]) outside of module 'foo' is not a good practice,
    because it makes it hard or impossible to understand the conditions
    when foo.c gets compiled. We have already two modules 'fopen' and
   'fopen-gnu' that do AC_LIBOBJ([foo]); this is hard enough to understand.

The fact that you needed to duplicate the logic of

  if test $REPLACE_FCLOSE = 1; then
    REPLACE_FOPEN=1

is an indication of a workaround to problem (1).

For solving problem (1), Autoconf offers us AC_REQUIRE; so that's what we
should use.

It is reasonably maintainable, on the other hand, to have a module depend
on the results of the configuration of another module, that is, to *inspect*
the value of REPLACE_FOO of module 'foo'. For example,
  - fclose.m4 does AC_REQUIRE([gl_FUNC_CLOSE]) and then tests REPLACE_CLOSE.
    That is fine.
  - fclose.m4 calls gl_FUNC_FFLUSH_STDIN, which exists in fflush.m4. That
    is also fine.

I'm therefore moving the "if REPLACE_FCLOSE is 1, set REPLACE_FOPEN to 1"
logic to module 'fopen'. You'll notice that the logic of

  if test $REPLACE_FCLOSE = 1; then
    REPLACE_FOPEN=1

is now present in a single place only.


2023-04-26  Bruno Haible  <bruno@clisp.org>

        fclose: Make last change more maintainable.
        * m4/fclose.m4 (gl_FUNC_FCLOSE): Define through AC_DEFUN_ONCE. Don't
        modify REPLACE_FOPEN.
        * modules/fclose (Depends-on): Add comment.
        (configure.ac): Don't modify REPLACE_FOPEN. Don't duplicate actions of
        module 'fopen'.
        * m4/fopen.m4 (gl_FUNC_FOPEN_ITSELF): Renamed from gl_FUNC_FOPEN.
        (gl_FUNC_FOPEN): New macro.
        * modules/fopen (Files): Add m4/fclose.m4, m4/fflush.m4.
        * m4/close.m4 (gl_FUNC_CLOSE): Define through AC_DEFUN_ONCE.

diff --git a/m4/close.m4 b/m4/close.m4
index 9f95c670e5..0feabd6917 100644
--- a/m4/close.m4
+++ b/m4/close.m4
@@ -1,10 +1,10 @@
-# close.m4 serial 9
+# close.m4 serial 10
 dnl Copyright (C) 2008-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.
 
-AC_DEFUN([gl_FUNC_CLOSE],
+AC_DEFUN_ONCE([gl_FUNC_CLOSE],
 [
   AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
   m4_ifdef([gl_MSVC_INVAL], [
diff --git a/m4/fclose.m4 b/m4/fclose.m4
index 74b7df0a77..e9291f0bda 100644
--- a/m4/fclose.m4
+++ b/m4/fclose.m4
@@ -1,10 +1,10 @@
-# fclose.m4 serial 10
+# fclose.m4 serial 11
 dnl Copyright (C) 2008-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.
 
-AC_DEFUN([gl_FUNC_FCLOSE],
+AC_DEFUN_ONCE([gl_FUNC_FCLOSE],
 [
   AC_REQUIRE([gl_STDIO_H_DEFAULTS])
   AC_REQUIRE([AC_CANONICAL_HOST])
@@ -31,10 +31,6 @@ AC_DEFUN([gl_FUNC_FCLOSE]
       *) REPLACE_FCLOSE=1 ;;
     esac
   fi
-
-  if test $REPLACE_FCLOSE = 1; then
-    REPLACE_FOPEN=1
-  fi
 ])
 
 dnl Determine whether fclose works on input streams.
diff --git a/m4/fopen.m4 b/m4/fopen.m4
index 6806394f8c..7daa4caec5 100644
--- a/m4/fopen.m4
+++ b/m4/fopen.m4
@@ -1,10 +1,10 @@
-# fopen.m4 serial 14
+# fopen.m4 serial 15
 dnl Copyright (C) 2007-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.
 
-AC_DEFUN([gl_FUNC_FOPEN],
+AC_DEFUN([gl_FUNC_FOPEN_ITSELF],
 [
   AC_REQUIRE([gl_STDIO_H_DEFAULTS])
   AC_REQUIRE([AC_CANONICAL_HOST])
@@ -58,6 +58,15 @@ AC_DEFUN([gl_FUNC_FOPEN]
   esac
 ])
 
+AC_DEFUN([gl_FUNC_FOPEN],
+[
+  AC_REQUIRE([gl_FUNC_FOPEN_ITSELF])
+  AC_REQUIRE([gl_FUNC_FCLOSE])
+  if test $REPLACE_FCLOSE = 1; then
+    REPLACE_FOPEN=1
+  fi
+])
+
 AC_DEFUN([gl_FUNC_FOPEN_GNU],
 [
   AC_REQUIRE([gl_FUNC_FOPEN])
diff --git a/modules/fclose b/modules/fclose
index 9a949e48b8..0ef34bdfd9 100644
--- a/modules/fclose
+++ b/modules/fclose
@@ -9,18 +9,16 @@ Depends-on:
 stdio
 close           [test $REPLACE_FCLOSE = 1]
 fflush          [test $REPLACE_FCLOSE = 1]
-fopen           [test $REPLACE_FCLOSE = 1]
 freading        [test $REPLACE_FCLOSE = 1]
 lseek           [test $REPLACE_FCLOSE = 1]
 msvc-inval      [test $REPLACE_FCLOSE = 1]
+# The code of fclose does not need to call fopen. But when gcc's '-fanalyzer'
+# option is in use and stdio.h does '#define fclose rpl_fclose', stdio.h also
+# needs to do '#define fopen rpl_fopen', otherwise a warning may result.
+fopen           [test $REPLACE_FCLOSE = 1]
 
 configure.ac:
 gl_FUNC_FCLOSE
-if test $REPLACE_FCLOSE = 1; then
-  REPLACE_FOPEN=1
-  AC_LIBOBJ([fopen])
-  gl_PREREQ_FOPEN
-fi
 gl_CONDITIONAL([GL_COND_OBJ_FCLOSE], [test $REPLACE_FCLOSE = 1])
 gl_STDIO_MODULE_INDICATOR([fclose])
 
diff --git a/modules/fopen b/modules/fopen
index da216c9cec..3d538393f1 100644
--- a/modules/fopen
+++ b/modules/fopen
@@ -4,6 +4,8 @@ fopen() function: open a stream to a file.
 Files:
 lib/fopen.c
 m4/fopen.m4
+m4/fclose.m4
+m4/fflush.m4
 
 Depends-on:
 stdio