Skip to main content
5 of 5
added 1 character in body
J_H
  • 42.3k
  • 3
  • 38
  • 157

specification

make shalt build ...

Writing down requirements in English can be very helpful, keep doing it. But please spell it "shall". In KJV it appears fifteen hundred times, in fourteen hundred of them next to "thou". But "thou" is going to look a little funny in a modern spec.

conventional targets

make tclean

This is a pretty OK target name, you can certainly keep it.

Consider adopting a more broadly used target name seen in other packages, such as clean-all or very-clean. Then even without consulting documentation, a maintenance engineer will immediately understand that this is "more destructive" and should take care, perhaps should even consult the docs or the source or use make -n, before blindly plunging ahead with it.

OTOH these five PHONYs are perfect:

  • all
  • valgrind
  • benchmark
  • test
  • clean

standard

CFLAGS += -std=gnu2x

I know, we're using GCC. And maybe there is a good reason for needing this. But for new projects you might consider habitually asking for ISO compliance, e.g. c17 or c2x. Go with it till you hit a speedbump that makes gnu2x look attractive. And then you'll be able to write down a comment for future maintainers explaining which GNU feature we're relying on.

Generally the "pedantic plus others" looks great. It's unclear to me why TEST_CFLAGS drops most of them. And I assume there was some "sanitize" trouble.

surprising switches

RM = /bin/rm -r
MKDIR = /bin/mkdir -p
...
clean:
    $(RM) $(TARGET) $(OBJ_DIR)

Please don't do that.

I suppose the -p is harmless enough. But that -r is a land mine waiting to go off. This is a POLA violation. Down at the call site (the clean recipe) we should be explicitly supplying a -r recursive argument, for clarity to anyone working with this. Hiding it way at the top gives misleading signals to someone reading the recipe.

OTOH you might consider putting repeated flags into VALGRIND or VALGRIND_FLAGS.

Also, please don't hardcode full pathnames; prefer to use $PATH. On my ubuntu jammy server, it happens that /bin is symlinked down into /usr/bin. But there are lots of machines where /bin is on the root filesystem so it's available during fsck before /usr has been mounted. There's just no reason to include a full pathname.

propagate the dep

Rather than hardcoding the binary name in a bash script

benchmark: $(TARGET)
    ./benchmark > log.txt

you might consider writing ./benchmark $(TARGET) > log.txt, and then have bash pick it out from argv.

test deps

reduce the duplication that the test target has

Yeah, I agree, that does look kind of nightmarish.

First thought on this is to simply throw *.o into a test.a archive. Including common.o. And then build executable binaries as needed.

But then we have the {FEOF, RESIZE_FBUF, ALLOC_AND_APPEND_LINE} stuff to worry about. Maybe that doesn't quite belong here? Maybe deal with it in .c and .h source code? And then the Makefile need only worry about these objects:

  • readlines_fread_test_failure_feof.o
  • readlines_fread_test_failure_resize_fbuf.o
  • readlines_fread_test_failure_alloc_and_append_line.o

It would be simple to write rules to build the associated executables.

I'm thinking we could have three very short *.c files for those three variants, which simply #define and then #include the bulk of the source.

Flags like ALLOC_AND_APPEND_LINE appear repeatedly. Trading efficiency for simplicity, would it perhaps make any sense to gather up a list of all flags, and rebuild everything once per flag, re-using a single executable name? Ok, prolly not, just a thought. More realistically, perhaps define five option lists with 3 to 4 -D defs in each, and loop over those lists to produce five test run results.

J_H
  • 42.3k
  • 3
  • 38
  • 157