cloudflare / keyless

Cloudflare's Keyless SSL Server Reference Implementation
Other
276 stars 78 forks source link

Remove unnecessary null pointer checks #77

Closed elfring closed 9 years ago

elfring commented 9 years ago

An extra null pointer check is not needed in functions like the following.

Would you like to apply the following semantic patch to find more update candidates?

@Remove_unnecessary_pointer_checks@
expression x;
@@
-if (x)
    free(x);
jgrahamc commented 9 years ago

Happy to merge a PR for this.

elfring commented 9 years ago

Why did you close this bug report before the affected source code will eventually be improved?

jgrahamc commented 9 years ago

I closed it because there's no 'bug' here. Sure we don't need to check for null before calling free(), but it has no effect on the code. If you want to fix all the instances and make a PR I'll merge it.

elfring commented 9 years ago

Unnecessary checks can have unwanted run time consequences.

How do you think about to try out the tool "Coccinelle" also on your own for such a simple use case?

jgrahamc commented 9 years ago

What unwanted run time consequence is

if (p) {
    free(p);
}

going to have over

free(p);

?

elfring commented 9 years ago

Do implementation details like the following matter here?

nitrix commented 9 years ago

I prefer free(p) to if (p) free(p); as well. There's no reason for extra code already covered by the language semantics.

jgrahamc commented 9 years ago

Fixed.

elfring commented 9 years ago

Thanks for your small source code improvement.