some snippets from make381, 2/2

[Markus Mauhart]

"Markus Mauhart" wrote ...
>
> during some long ago experiments with make381beta1 sources I've
> gathered some questionable code snippets, some maybe bugs, but
> didnt find the time to discuss them, now I thought better now
> than even later.
> If necessary I can file something to savannah.
> A few others are still to be extracted from my archive.

And here comes the rest. Not included are VMS-specifics, but IIRC
the vms*-files contain great opportunities for innovation ;-)


Regards,
Markus.


*****************************
* bool, filedef.h, implicit.c, dep.h:

"struct file" contains the following bitfield, which is intended as bool:

  unsigned int tried_implicit:1;

Such pseudo-bool is very dangerous, cause even " |= 0xfffffe" wouldnt change
the bit. What I mean is line 819 in implicit.h:

    dep->file->tried_implicit |= dep->changed;
    //Fail-save code:
    //if (dep->changed) dep->file->tried_implicit = true;

Who knows whithout expensive checks whether this does the right thing ?
I did expensive checks and still dont know.
'dep->changed' referres to "struct dep"s member "unsigned int changed : 8;"
Here my old comment says:
  /* FIXME: sometimes CHANGED is used as RM_xxx-flag set, sometimes as a single 
bool "flag". */
  /* Problem is that both sorts of "sometimes" are not documented, which makes 
reliable */
  /* and verifyable usage impossible. So either add this docu, or replace 
CHANGED with */
  /* two separate items, e.g. [union] {byte rmflags ;bool changed ;} */
If nobody can verify the above "|=", IMHO one should replace it with save code.
Btw, see below w.r.t. remake.c, it really uses the 8-bit dep->changed
as a kind of counting bool.
"struct dep" also contains member "unsigned int ignore_mtime : 1;"
And "struct idep" contains member "unsigned char ignore_mtime;"
AFAICS they are used correctly, nevertheless I'd replace both with type 'bool',
which would make human checks redundant and wont cost a single byte at
runtime (cause of alignment induced padding).


*****************************
* file.c, filedef.h, lookup_file(xptr)

    extern struct file *lookup_file PARAMS ((char *name));

My copy contains:

    extern struct file *lookup_file PARAMS ((char const *name));
        /* often called with string litteral as name, e.g. ".PHONY" */

lookup_file's implementation actually DOES treat its argument as-if "const",
nevertheless to please the compiler's const warning one has to change a few
trivial things:
=> my completely equivalent version of lookup_file(xptr), but with
xptr correctly declared as "const":

    struct file *
    lookup_file (char const *name)
    {
      struct file *f;
      struct file key;

      assert (*name != '\0');

      /* This is also done in parse_file_seq, so this is redundant
        for names read from makefiles.  It is here for names passed
        on the command line.  */
    #ifdef VMS
    # ifndef WANT_CASE_SENSITIVE_TARGETS
      {
        char const* src = name;
        char*       dst = (char*) alloca (strlen(src) + 1);
        name = dst;
        for ( ; *src != '\0'; ++src, ++dst)
          *dst = isupper ((unsigned char)*src) ? tolower ((unsigned char)*src) 
: *src;
        *dst = '\0';
      }
    # endif

      while (name[0] == '[' && name[1] == ']' && name[2] != '\0')
          name += 2;
    #endif
      while (name[0] == '.' && name[1] == '/' && name[2] != '\0')
        {
          name += 2;
          while (*name == '/')
            /* Skip following slashes: ".//foo" is "foo", not "/foo".  */
            ++name;
        }

      if (*name == '\0')
        /* It was all slashes after a dot.  */
    #ifdef VMS
        name = "[]";
    #else
    #ifdef _AMIGA
        name = "";
    #else
        name = "./";
    #endif /* AMIGA */
    #endif /* VMS */

      key.hname = (char*) name;
      f = (struct file *) hash_find_item (&files, &key);
      return f;
    }
*******************************
* file.c, enter_file(xptr):

my comment said:
/* Was a bug ... 381beta1 contained such obfuscated VMS handling so that
   btw it removed "free(name)" even for the non-VMS case.
*/
AFAICS the original VMS-code seems to have missed that the function-argument
is completely under control of the function -> may be lowercase'd inline.
Wait, now I see that the reason for enter_file's unnecessary VMS-malloc is that
enter_file has been copied&pasted from lookup_file - but lookup_file(xptr)
has semantic so that xptr can be (and often is) a literal (".PHONY"),
while enter_file's xptr has opposite semenatic.

My replacement for the whole function was ...

  struct file *
  enter_file (char *name)
  {
    struct file **slot;

    assert (*name != '\0');

  #if defined(VMS) && !defined(WANT_CASE_SENSITIVE_TARGETS)
    {
      char *a;
      for (a = name; *a != '\0'; ++a)
        if (isupper ((unsigned char)*a))
          *a = tolower ((unsigned char)*a);
    }
  #endif

    /* 1) Find SLOT of existing or still-to-create FILES entry: */
    {
      struct file key;

      key.hname = name;
      slot = (struct file **) hash_find_slot (&files, &key);
      if (! HASH_VACANT (*slot) && !(*slot)->double_colon)
        {
          free (name);
          return *slot;
        }
    }

    /* 2) Put new entry into SLOT (or into *SLOT's list): */
    {
      struct file *a;

      a = (struct file *) xmalloc (sizeof (*a));
      bzero ((char *) a, sizeof (*a));
      a->name = a->hname = name;
      a->update_status = -1;

      if (HASH_VACANT (*slot))
        hash_insert_at (&files, a, slot);
      else
        {
          /* There is already a double-colon entry for this file.  */
          struct file *f = *slot;

          a->double_colon = f;
          while (f->prev != 0)
            f = f->prev;
          f->prev = a;
        }

      return a;
    }
  }

**********************************
* file.c

    char *
    build_target_list (char *value)
    {
      static unsigned long last_targ_count = 0;

      if (files.ht_fill != last_targ_count)

My version says ...

      if (files.ht_fill != last_targ_count) /* FIXME ... is this really the 
correct indicator ? */

... maybe only a dumb question, but who knows.

**********************************
* file.c:

line 67 defines a currently unused macro ...

    #ifndef FILE_BUCKETS
    #define FILE_BUCKETS 1007
    #endif

... which looks like a perfect replacement for "1000" in line 925:

    void
    init_hash_files (void)
    {
      hash_init (&files, 1000, file_hash_1, file_hash_2, file_hash_cmp);
    }

*******************
* bool, job.*:

"struct child" has member "unsigned int remote:1;", again an optimized bool.

remote-stub.c contains:

    /* Return nonzero if the next job should be done remotely.  */
    int
    start_remote_job_p (int first_p UNUSED)
    {
      return 0;
    }

job.c, line 822, combines both:

    c->remote = start_remote_job_p (0);

So IMHO either make the variable and the function-return 'bool', or say ...

    c->remote = (start_remote_job_p (0) != 0);

*******************
* bool, job.*, commands.h

commands.h(37): #define COMMANDS_NOERROR 4

job.h: "struct child" has member "unsigned int noerror:1;"

job.c:
    flags = (child->file->command_flags
            | child->file->cmds->lines_flags[child->command_line - 1]);

    p = child->command_ptr;
    child->noerror = flags & COMMANDS_NOERROR;

IMHO the last assignment is even optimized away by GCC.
Ok, this now looks so unbelievable wrong for me that probably I'm
missing something and I soon will make a break ;-)

job.c, line 1230:
    child->remote = is_remote;
In a bool-less world better say ...
    child->remote = (is_remote != 0);

line 2860: "if", "if"

**********************
* main.c

line 602: switch (tolower (p[0]))
Not knowing p's details I would write ...
          switch (tolower ((unsigned char) p[0]))

line 1581: extern int unixy_shell;
... this is also declared in make.h.

************************
* bool, make.h, configure.in:

IMHO both humans and today's compilers greatly profite from dealing with
_Bool (C99) or bool (c++) instead of unsigned (int|char) [:n].
So to ease future development, according to autoconf.html only a little
bit must be copied&pasted into make.h and configure.in:
configure.in needs AC_HEADER_STDBOOL, make.h needs ...
  #if HAVE_STDBOOL_H
  # include <stdbool.h>
  #else
  # if ! HAVE__BOOL
  # ifdef __cplusplus
  typedef bool _Bool;
  # else
  typedef unsigned char _Bool;
  # endif
  # endif
  # define bool _Bool
  # define false 0
  # define true 1
  # define __bool_true_false_are_defined 1
  #endif

**************************************
* read.c

(this is only w.r.t. readability)

line 128 contains ...
  static int eval PARAMS ((struct ebuffer *buffer, int flags));

line 450 is a bit more concrete w.r.t. "flags":
  static int eval (struct ebuffer *ebuf, int set_default)

I've changed it to ...

  static void eval PARAMS ((struct ebuffer *buffer, bool set_default));

... because eval() does return 1 unconditionally, and its return value is
undocumented and not really used.

Also from eval_buffer(..) I've removed its return, cause it was
unconditionally 1, and not used.

**************************************
* read.c, "char* tilde_expand (char* name)"

At its end it does ...

  #if !defined(_AMIGA) && !defined(WINDOWS32)
    else
      {
        struct passwd *pwent;
        char *userend = strchr (name + 1, '/');
        if (userend != 0)
          *userend = '\0';
        pwent = getpwnam (name + 1);
        if (pwent != 0)
          {
            if (userend == 0)
              return xstrdup (pwent->pw_dir);
            else
              return concat (pwent->pw_dir, "/", userend + 1);
          }
        else if (userend != 0)
          *userend = '/';
      }
  #endif /* !AMIGA && !WINDOWS32 */
  #endif /* !VMS */
    return 0;
  }

I thought that it should restore its argument "name" in any case, hence I'd
put "if (userend != 0) *userend = '/';" immediately after
"pwent = getpwnam (name + 1);", before returning.
But maybe this effect is desired.

**********************************
* bool, remake.c
line 162:
    g->changed += commands_started - ocommands_started;

(g is struct dep's "unsigned int changed : 8;")

IMHO the only save method is ...

    if (commands_started != ocommands_started)
        g->changed = true;

**********************************
* w32/pathstuff.c

w32ify() misses to handle 2 errors, one only occurring with artificial
long filenames (e.g. produced by makefile-bugs), the other one IMHO looks
rather likely. Additionally w32ify() flushes the CPU cache considerably.
So I've replaced it completely:

  /*
  * Return pointer to internal buffer.
  * Convert to forward slashes. Resolve to full pathname optionally.
  */
  char *
  w32ify(char const *filename, bool resolve)
  {
      static char w32_path [FILENAME_MAX];
      char *p;

      if (resolve)
        {
          if (!_fullpath(w32_path, filename, sizeof (w32_path)))
            /* invalid filename syntax, or drive doesnt exist, or buffer too 
small.
            FIXME: another diagnostic ? */
            goto FAILED;
        }
      else
        {
          /*  ! Any "strncpy" writes FILENAME_MAX (e.g.1024) bytes 
unconditionally !
              And can create non-zero-terminated strings, and it has no error 
indicator. */
          size_t const a = 1 + strlen(filename);
          if (a > sizeof(w32_path))
            /* buffer too small. FIXME: another diagnostic ? */
            goto FAILED;
          memcpy (w32_path, filename, a);
        }

      for (p = w32_path; *p; p++)
          if (*p == '\\')
              *p = '/';

      return w32_path;

      FAILED:
      return (char*) memset (w32_path ,'\0' ,8);
          /*  Why not "w32_path[0] = 0" ?
              Cause some functions might assume that "w32ify(*,true)" couldnt 
fail
              and resulted unconditionally in things like "d:/rest". */
  }