#61 Corrected funcionality to bu_basename

[Guilherme Kunigami]

Function bu_basename() says it'll return things that it doesn't. For instance, the input "////" should return "/" and "a/" should return "a", but in both cases the current code is returning an empty string.

I edited the code to solve these problems. I had to include string.h lib and also allocate a new string since the input string may need to be modified.

I've attached the diffs in files include/bu.h and src/libbu/basename.c

[Guilherme Kunigami]

[Sean Morrison]

Guilherme, at a glance, the patch needs a little more work as it doesn't conform to at least one of our HACKING guidelines. It's a minor point, but you should be familiar with the guidelines since it's the first step towards attaining commit privs. The main issue is calling malloc(), so you can start there but I would suggest getting familiarized with everything in the guidelines.

If you want to take this patch to the next level, a regression test file would be useful -- one that calls bu_basename() and basename() for a variety of inputs testing common use and edge cases, to make sure they both return expected results. Not necessary, but definitely something that would be useful.

[Guilherme Kunigami]

I've updated the patch. I read the guidelines and I changed the code so that it conforms to it. This includes calling bu_strlcpy and bu_malloc instead of the standard ones. Also I corrected spacing around operators.

Following the recomendation, I added a uni-test to this function, namely basenametester.c at /src/libbu (I also included it the Makefile.am). It performs tests against unix basename (I assumed that noinst binaries shouldn't be cross plataform).

[Guilherme Kunigami]

updated patch

[Sean Morrison]

A quick review and the update looks much better. Curious that you used the basename binary with popen() instead of the basename(3) function in basenametester. Any particular reason?

That said, there is still one remaining issue. Now that bu_basename() is allocating memory on the heap, the return value should no longer be const and callers need to make sure that memory is released. Otherwise, applying the patch would inject memory leaks everywhere it's called.

[Guilherme Kunigami]

updated patch - version 3

[Guilherme Kunigami]

brlcad-v3.patch:

I've updated the patch once again. I didn't know that there was a basename function in c :) I've changed that.

I've tried to track down all calls to bu_basename on brlcad sources to avoid memory leak. I added a bu_free to almost all of them except on nirt/if.c file. There exists a external variable namely ValTab which points to the output of bu_basename. As far as I could check, it's not trivial to provide a free function to it or use static buffers.

[Sean Morrison]

Applied as of r44746, thanks! Your test harness made it easy to add a few more test cases that I expected might fail (and they did). Easily fixed, and working much better now.

Now to look into avoiding the malloc while remaining thread safe and reentrant.