Re: gnulib-tool.py: Fix removal of variables from GLMakefileTable.

[Bruno Haible]

Hi Collin,

> This patch fixes the extra line printed by gnulib-tool.py:
> 
> $ diff  -u test-oath-toolkit-4.out tmp758920-out
> --- test-oath-toolkit-4.out   2024-03-28 13:17:54.441485990 -0700
> +++ tmp758920-out     2024-03-28 14:36:28.290120583 -0700
> @@ -194,5 +194,6 @@
>    - mention "gl" in SUBDIRS in Makefile.am,
>    - mention "-I gl/m4" in ACLOCAL_AMFLAGS in Makefile.am
>      or add an AC_CONFIG_MACRO_DIRS([gl/m4]) invocation in ./configure.ac,
> +  - mention "m4/gnulib-cache.m4" in EXTRA_DIST in gl/Makefile.am,
>    - invoke gl_EARLY in ./configure.ac, right after AC_PROG_CC,
>    - invoke gl_INIT in ./configure.ac.
> 
> The removal of variables from the GLMakefileTable was incorrect. The
> '.pop()' call was on a local variable dictionary. I fixed it by adding
> a pop function taking the index to GLMakefileTable.

This part looks good.

> Since we need the GLMakefileTable between function calls, I've added
> it as an instance variable to GLEmiter. This also means removing it
> from other classes which passed it as an argument.
> 
> It has been a while since I've used OOP, but this relationship seems
> more correct to me anyways.

This part does not look so good.

Since the program uses a mix between OOP and functional style, it is
reasonable to think about whether a certain data path is better implemented
through OOP (with an object holding a private sub-object) or better done
in a functional style (by passing function parameters).

When doing so, two considerations should be evaluated:
  1) Is the sub-object really private?
  2) Do the sub-objects need to be shared? I.e. is it OK to have multiple
     instances? Or is it better to have one instance only?

In this case:

1) The lines in your new code

+            self.emitter.makefiletable.editor(sourcebase_dir, 'SUBDIRS', 
sourcebase_base)
+            self.emitter.makefiletable.editor(pobase_dir, 'SUBDIRS', 
pobase_base)
+                self.emitter.makefiletable.editor(testsbase_dir, 'SUBDIRS', 
testsbase_base, True)
+        self.emitter.makefiletable.editor('', 'ACLOCAL_AMFLAGS', m4base)
+        self.emitter.makefiletable.parent(gentests, source_makefile_am, 
tests_makefile_am)

prove that the GLImport object needs to access the GLMakefileTable, thus the
GLMakefileTable is *not* a private part of the GLEmiter. This goes against the
OOP principles.

2) Before, it was OK to have one GLEmiter or several GLEmiters with the
same GLConfig. Now, it matters. This is not fatal; it can be mitigated
through comments. But it is something to watch out.

But 1) convinces me that the original style, with the GLMakefileTable as an
explicit constructor argument, was better (in terms of maintainability).

One could attempt to solve 1) by adding to the GLEmiter an 'editor' method
and a 'parent' method that delegate to the embedded GLMakefileTable. The
question is, though, whether the GLMakefileTable then still is private — or
whether you have opened too much of its API... I would say, it's the latter.

That is, I would prefer to revert that part, and keep the GLMakefileTable
as an explicit argument.

Bruno