"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". */
}
_______________________________________________
Bug-make mailing list
Bug-make@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-make
