db.cc: condition is always true

[frederic-mahe]

https://github.com/torognes/swarm/blob/10f1d85d6233105d124facee21f97ca52f8ce849/src/db.cc#L211

const char * attribute = "size=";
const char * digit_chars = "0123456789";

if ((! header) || (! attribute))
    return false;

Isn't attribute always set? Hence, the second part of the condition (or not attribute) is not necessary?

[frederic-mahe]

For example, passing a empty header does not trigger the return false (line 212):

(gdb) run -z <(printf ">\nA\n")
Breakpoint 3, find_usearch_abundance (header=0x7ffff799c014 "", start=0x7fffffffd7ac, end=0x7fffffffd7a8, number=0x7fffffffd7a0) at db.cc:211
211   if ((! header) || (! attribute))
(gdb) print header
$3 = 0x7ffff799c014 ""
(gdb) s
214   uint64_t hlen = strlen(header);
(gdb) 

[torognes]

Yes, attribute is always set, so the second part of the condition is always true. I will remove it.

Regarding the example: An empty string is still a string (not a zero pointer) so header will still be set.

[torognes]

Fixed now.

[torognes]

The check is meant to capture internal coding errors, not empty headers.

[frederic-mahe]

The check is meant to capture internal coding errors, not empty headers.

Thanks, it makes sense. Then, would it be a good opportunity to use an assert?

assert(header) /* else something is very wrong */

If I understand correctly, assertions are not evaluated at runtime, unless compiled with NDEBUG. So there is no performance penalty when using assert(), where there can be some when using conditionals branching to capture coding errors.

[torognes]

Maybe assert would be fine here, but the overhead is negligible and I prefer the function to return false if the parameters are illegal.

[frederic-mahe]

That makes sense. Thanks!