Remove unnecessary null pointer checks

[elfring]

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

• r_anal_diff_free
• r_asm_code_free
• zip_discard

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

@Remove_unnecessary_pointer_checks@
expression x;
@@
-if (x)
    free(x);

[XVilka]

I think it was added because of coverity bitching about that.

[radare]

Those if’s before free() should be removed. this is from 3rd party contributors. i try to remove them all, but looks like i failedl at catching them all.

On 24 Mar 2014, at 14:52, Markus Elfring notifications@github.com wrote:

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

r_anal_diff_free r_asm_code_free zip_discard Would you like to apply a semantic patch like the following to find more update candidates?

@Remove_unnecessary_pointer_checks@ expression x; @@ -if (x) free(x); — Reply to this email directly or view it on GitHub.

[condret]

There are a lot of other useless things, like foo=R_NEW(RFoo) + memset(foo, 0, sizeof(RFoo)) instead of foo=R_NEW0 (RFoo)

[radare]

Could you provide a patch?

[elfring]

@radare Who should suggest another concrete patch?

[radare]

I was asking for the pullrequest with the patch changing all those rnew and if/free

On 26 Mar 2014, at 10:17, Markus Elfring notifications@github.com wrote:

@radare Who should suggest another concrete patch?

— Reply to this email directly or view it on GitHub.

[elfring]

How do you think about to try out semantic patches here?

[radare]

I find semantics patches interesting. As well as defining the indentation rules in the uncrustify config file to indent all the code following the same rules.

Didnt tried tools like cocinella before

On 26 Mar 2014, at 11:16, Markus Elfring notifications@github.com wrote:

How do you think about to try out semantic patches here?

— Reply to this email directly or view it on GitHub.

[XVilka]

@radare I've tried them. They even allows to convert intel graphics driver from linux kernel to the vga rom for the coreboot. Awesome power!

[elfring]

Would you like to fiddle a bit more with the tool "spatch"?

[condret]

yes, I just made some changes to op.c, xrefs.c, value.c and r_anal.h

We should not use memcpy for copying structs. Instead we should copy them directly. We should look out for snprintf()'s which can be replaced by sprintf() if we know, that the source has a constant length

[radare]

Using sprintf results on warnings by coverity and scan analyzer. Agree with copying structs.

Also... Can we write automatic source translations to solve compiler warnings like missing semicolons, automatic casts and other magic shit?

On 26 Mar 2014, at 11:51, condret notifications@github.com wrote:

yes, I just made some changes to op.c, xrefs.c, value.c and r_anal.h

We should not use memcpy for copying structs. Instead we should copy them directly. We should look out for snprintf()'s which can be replaced by sprintf() if we know, that the source has a constant length

— Reply to this email directly or view it on GitHub.

[elfring]

How do you think about to separate update candidates from static source code analysis into different open issues?

[deeso]

Can we close this issue?

[radare]

I removed two of them yesterday. Did you checked if there are more?

On 06 May 2014, at 06:10, dso notifications@github.com wrote:

Can we close this issue?

— Reply to this email directly or view it on GitHub.

[elfring]

Would you like to improve three functions I mentioned in my initial request?

[jvoisin]

@elfring Did you tried to run coccinelle on the codebase ?

[elfring]

@jvoisin: I could try out the tool "spatch" on your source files again. Which details would you really like to know?

[deeso]

So this is more of an aesthetic quality and it does not impact performance, functionality, or even readability. We can all agree they are unnecessary, but this type of issue only raises the fact that we dont have time to make code pretty.

On Tue, May 6, 2014 at 7:50 AM, Markus Elfring notifications@github.comwrote:

@jvoisin https://github.com/jvoisin: I could try out the tool "spatch" on your source files again. Which details would you really like to know?

— Reply to this email directly or view it on GitHubhttps://github.com/radare/radare2/issues/725#issuecomment-42297707 .

PhD Student, Rice University Office: Duncan Hall 1083 Office Hours: M 11-12p

[elfring]

@deeso: Unnecessary checks have got unwanted effects on run time behaviour. Their relevance will vary with the call frequency for the affected functions.

[jvoisin]

@elfring Just send a PR with changes :)

Also, it would be interesting to have this kind of patches in a Makefile, to be run once a while on the whole codebase.

[elfring]

@jvoisin: Would you like to integrate the suggested semantic patch into your software build process?

[jvoisin]

This would make coccinelle a requirement for building. Plus, the goal is to have those changes integrated into the codebase, not to get them only in the freshly built binary.

[elfring]

I would appreciate if patches can also be expressed in a semantic way.

[deeso]

@elfring I appreciate your insight into the performance impact, and if radare was run as an operating, system, hypervisor, and anything required to be performant I would agree with you.

This is a user driven hex-editor that has bigger bottlenecks than null checks. Unfortunately, none of your proposals have really met the projects needs. We can't integrate the software at this point in time, and we do not have the resources to realistically go back and make semantic changes you are proposing.

We appreciate your opinion and recommendations, but unless you can come up with a pull-request that addresses these issues or you can create an external process that makes these issues semantically correct and creates that pull-req, this issue will probably be closed as won't fix.

[jvoisin]

I think that making a PR, or adding a target in the build process will be enough :)

[deeso]

Yes this would be fantastic and very helpful. @elfring can you do this for us?

[elfring]

Source code adjustments can be generated from semantic patches also in a format you are more used to so far.

By the way: The Linux build infrastructure provides a target "cocci_check". Do you see a need for a similar one with your software?

[jvoisin]

It would be a nice way to check for bugs/errors. His is what I was talking about in my previous message.

[elfring]

I am not familiar enough with your build tool "autoconf replacement" to add a special target quickly.

[radare]

@elfring wrong thread?

[elfring]

@radare: Do you want to continue the clarification for this issue with a different communication channel?

[jvoisin]

Any news about this, or shall we close this bug because no one cares anymore?

[XVilka]

Extracted coccinelle check from this, closing it.

[elfring]

@XVilka: Where did you store this extract?

[XVilka]

@elfring lol, it is here https://github.com/radare/radare2/issues/1646

[elfring]

@XVilka: It seems that you would like to continue the discussion on a different topic then. Will my initial update suggestion be clarified (or improved) later?

[XVilka]

@elfring ofc it is possible, so someone, or you just reopen this bug, or just send a pull request.