Re: libiberty patch for gdb cross-debugging support

[DJ Delorie <dj at redhat dot com>]

Commenting only on the libiberty parts...

filesystem.c is missing.

> +/* Defined in gdb/top.c

libiberty can't depend on functions in gdb, likely the comment is just
wrong.

> +extern int have_dos_based_file_system(void);

GNU coding conventions require a space between function names and the
paren.

> +#define	_isalpha(c) (((c >= 'A') && (c <= 'Z')) || ((c >= 'a') && (c <= 'z')))

This is wrong on EBCDIC hosts, and even more wrong when the host and
target have different encodings.

> +	(have_dos_based_file_system() ? (_isalpha(f[0]) && (f[1] == ':')) : 0))

This is wrong for DOS paths (drive letters are not always letters!)

> +extern int have_dos_based_file_system(void);

If we're doing this, we should separate the *host* filesystem needs
from the *target* filesystem needs.  I'm not sure if we want to open
that can of worms just yet, though, so let's not ;-)

> +	((IS_DIR_SEPARATOR(f[0])) ? 1 : \
> +	(have_dos_based_file_system() ? (_isalpha(f[0]) && (f[1] == ':')) : 0))

Can we avoid the ?: syntax here and just use || ?

	(IS_DIR_SEPARATOR((f)[0]) || \
	 ((f)[0] && ((f)[1] == ':') && have_dos_based_file_system()))

Note parens around macro argument uses.

> +#define	IS_DIR_SEPARATOR(c)	\
> +		((have_dos_based_file_system()) ? \
> +		((c == '/') || (c == '\\')) : \
> +		(c == '/'))

For performance, we should check '/' first, then '\\', *then* call
have_*().  That way when the optimizers short-circuit the tests, the
expensive ones become very unlikely to run at all.

#define	IS_DIR_SEPARATOR(c) \
	((c) == '/'
	|| ((c) == '\\' && have_dos_based_file_system()))

That way we avoid a function call for all non-slashish characters.

> -	./fnmatch.o ./fopen_unlocked.o					\
> +	./fnmatch.o ./fopen_unlocked.o ./filesystem.o			\

Alphabetical order, please.

> +./filesystem.o: $(srcdir)/filesystem.c

Did you run "make maint-deps" ?

> +  if (have_dos_based_file_system() && ISALPHA (name[0]) && name[1] == ':') 

Likewise, do the fast checks first, and don't ISALPHA.