A
Alan Curry
To make up for posting in one of those threads of mass stupidity, here's
something less off-topic.
I downloaded the library from http://code.google.com/p/ccl/ and here's what
I've noticed so far. This part is mostly about superficial packaging issues.
The meaty stuff is harder to comment on since I'd have to read the
documentation, and there's over 200 pages of that.
unzipping splattered all over current directory. This makes a bad first
impression. Can I apply tarball etiquette to zip files? Well, I just did.
There was a mention of ccl.tex but I don't see it anywhere.
The makefile actually worked, and is readable, instead of being a maze of
$(shell stuff) and includes and other non-POSIX features. That's rare these
days. However, when I tried it with a pmake (derived from NetBSD make) it
failed. The reason: line-continuing backslashes are not immediately followed
by newline. They have ^M's after them. "UNIX" makefile shouldn't have MS-DOS
line endings.
The ^M's in the C source files aren't fun to look at either.
The build rule for dotest should use test.o, not test.c. The way you have it,
test.c gets compiled with the implicit .c.o rule since test.o is one of OBJS,
then test.o never gets used, because the link line of dotest compiles it
again (without your CFLAGS!). Also, having test.o in OBJS means it's included
in the .a which it probably shouldn't be. To fix all that, just remove test.o
from OBJS and change the 2 instances of test.c in the dotest rule to test.o
Header file dependencies are not expressed in the Makefile, so nothing gets
rebuilt if a header file is modified. That's bad.
A few warnings were issued with gcc 4.4.5 and the options in the Makefile.
I'll go through them all:
In test.c there are a couple of %lu printf's with size_t args - gcc warns
about this as a potential mismatch. It should be %zu (clean, C99) or cast to
unsigned long (icky, C89)
The ULL suffixed integer constants in bitstrings.c give warnings because
-pedantic without -std=c99 is asking for C89 pedanticness, which disallows
long long.
Dead code here: bitstrings.c:503: warning: 'AddRange' defined but not used
More dead code here: searchtree.c:621: warning: 'hide' defined but not used
And here's one that's a serious behavioral issue: searchtree.c in the
definition of struct tagBinarySearchTreeNode has "char factor;" which is used
to hold the values LEFT, BALANCED, and RIGHT defined as 1, 0, and -1. On my
platform, char is unsigned so assigning -1 to a char makes it 255, and then
comparing it to -1 is false. So all the ==RIGHT tests are useless. gcc didn't
warn about those because you didn't include -W (-Wextra), but luckily it
warned about the switch() statements with case RIGHT.
Your test program segfaults because it gets severely confused by that
searchtree factor bug. After changing "char factor;" to "signed char factor;"
it didn't segfault. I can't interpret the test output so I won't say it
definitely passed all the tests, but at least it returned a 0 exit code.
One more formatting thing: it looks like you use tabstop=4. Lots of things
look wrong with tabstop=8, the default in unix land. If you used tabs
consistently this wouldn't be a problem but there are lots of places where a
line indented with a tab is followed by a line indented with 4 spaces, so
they don't line up for anyone who hasn't figured out that you wanted the
files to be viewed with tabstop=4.
Adding my own warning options, I get a lot more warnings. The most
interesting ones led me to:
casting away const in test.c:testDictionary on this line:
pi = (int *)iDictionary.GetElement(d,(const unsigned char *)"Two");
where GetElement returns const void *. This cast is not doing anything but
throwing away the const. This is occasionally justifiable with complex
interactions where the correct constness is situation-dependent, but this
isn't one of those cases. Nothing attempts to modify *pi so there are no
drawbacks to making pi a "const int *", and one big advantage: the cast can
go away. There are lots of other bad casts losing const for no reason; this
is just the one I picked as a representative of the group.
A lot of casts seem to be present because there are strings being passed
around as unsigned char * and then they get converted back to char * for use
with strcmp and such. I'd try to get rid of the "unsigned" stuff except where
it really matters. A function that wants to operate on a string of unsigned
chars internally should still have its arguments declared as plain "char *"
if that makes the caller's job easier.
Then there's the general lack of const in the proper places. A function
called Strcmp that wraps strcmp should really be able to take
pointer-to-const parameters. Omitting the const is just screwing up the
interface.
In buffer.c:CBClear, gcc thinks the variable p "may be used uninitialized"
and without studying too deeply, I can't disagree. It looks like the
memset(p, ...) is reachable with p uninitialized in the case where
cb->DestructorFn is false.
The big picture: count me among those not convinced that "container" is a
useful abstraction. Things indexed by integers and things indexed by strings
don't have that much in common.
part 2, later, if I have time to read the documentation.
something less off-topic.
I downloaded the library from http://code.google.com/p/ccl/ and here's what
I've noticed so far. This part is mostly about superficial packaging issues.
The meaty stuff is harder to comment on since I'd have to read the
documentation, and there's over 200 pages of that.
unzipping splattered all over current directory. This makes a bad first
impression. Can I apply tarball etiquette to zip files? Well, I just did.
There was a mention of ccl.tex but I don't see it anywhere.
The makefile actually worked, and is readable, instead of being a maze of
$(shell stuff) and includes and other non-POSIX features. That's rare these
days. However, when I tried it with a pmake (derived from NetBSD make) it
failed. The reason: line-continuing backslashes are not immediately followed
by newline. They have ^M's after them. "UNIX" makefile shouldn't have MS-DOS
line endings.
The ^M's in the C source files aren't fun to look at either.
The build rule for dotest should use test.o, not test.c. The way you have it,
test.c gets compiled with the implicit .c.o rule since test.o is one of OBJS,
then test.o never gets used, because the link line of dotest compiles it
again (without your CFLAGS!). Also, having test.o in OBJS means it's included
in the .a which it probably shouldn't be. To fix all that, just remove test.o
from OBJS and change the 2 instances of test.c in the dotest rule to test.o
Header file dependencies are not expressed in the Makefile, so nothing gets
rebuilt if a header file is modified. That's bad.
A few warnings were issued with gcc 4.4.5 and the options in the Makefile.
I'll go through them all:
In test.c there are a couple of %lu printf's with size_t args - gcc warns
about this as a potential mismatch. It should be %zu (clean, C99) or cast to
unsigned long (icky, C89)
The ULL suffixed integer constants in bitstrings.c give warnings because
-pedantic without -std=c99 is asking for C89 pedanticness, which disallows
long long.
Dead code here: bitstrings.c:503: warning: 'AddRange' defined but not used
More dead code here: searchtree.c:621: warning: 'hide' defined but not used
And here's one that's a serious behavioral issue: searchtree.c in the
definition of struct tagBinarySearchTreeNode has "char factor;" which is used
to hold the values LEFT, BALANCED, and RIGHT defined as 1, 0, and -1. On my
platform, char is unsigned so assigning -1 to a char makes it 255, and then
comparing it to -1 is false. So all the ==RIGHT tests are useless. gcc didn't
warn about those because you didn't include -W (-Wextra), but luckily it
warned about the switch() statements with case RIGHT.
Your test program segfaults because it gets severely confused by that
searchtree factor bug. After changing "char factor;" to "signed char factor;"
it didn't segfault. I can't interpret the test output so I won't say it
definitely passed all the tests, but at least it returned a 0 exit code.
One more formatting thing: it looks like you use tabstop=4. Lots of things
look wrong with tabstop=8, the default in unix land. If you used tabs
consistently this wouldn't be a problem but there are lots of places where a
line indented with a tab is followed by a line indented with 4 spaces, so
they don't line up for anyone who hasn't figured out that you wanted the
files to be viewed with tabstop=4.
Adding my own warning options, I get a lot more warnings. The most
interesting ones led me to:
casting away const in test.c:testDictionary on this line:
pi = (int *)iDictionary.GetElement(d,(const unsigned char *)"Two");
where GetElement returns const void *. This cast is not doing anything but
throwing away the const. This is occasionally justifiable with complex
interactions where the correct constness is situation-dependent, but this
isn't one of those cases. Nothing attempts to modify *pi so there are no
drawbacks to making pi a "const int *", and one big advantage: the cast can
go away. There are lots of other bad casts losing const for no reason; this
is just the one I picked as a representative of the group.
A lot of casts seem to be present because there are strings being passed
around as unsigned char * and then they get converted back to char * for use
with strcmp and such. I'd try to get rid of the "unsigned" stuff except where
it really matters. A function that wants to operate on a string of unsigned
chars internally should still have its arguments declared as plain "char *"
if that makes the caller's job easier.
Then there's the general lack of const in the proper places. A function
called Strcmp that wraps strcmp should really be able to take
pointer-to-const parameters. Omitting the const is just screwing up the
interface.
In buffer.c:CBClear, gcc thinks the variable p "may be used uninitialized"
and without studying too deeply, I can't disagree. It looks like the
memset(p, ...) is reachable with p uninitialized in the case where
cb->DestructorFn is false.
The big picture: count me among those not convinced that "container" is a
useful abstraction. Things indexed by integers and things indexed by strings
don't have that much in common.
part 2, later, if I have time to read the documentation.
