DevHeads.net

Review Request: Avoid possible null pointer dereferences in khtml

Review request for kdelibs.

Description
The real changes are:
Avoid possible null pointer dereferences in khtml.
The common check
if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a) with null a. changed to
if (a) { if something(a)) do return bla(a) else blabla(a) }

As a side effect:
kate has removed a lot of tailing spaces in the edited files.
Avoid a possible crash checking the index limit before accesing the array.
Move some variables inside the #ifdef block where they are used

Diffs
khtml/css/css_valueimpl.cpp 3fb2898
khtml/ecma/kjs_window.cpp 0e7394b
khtml/html/htmltokenizer.cpp b64e83d
khtml/java/kjavaappletserver.cpp 234c6f3
khtml/khtml_part.cpp 53929fa
khtml/khtmlimage.cpp c6e6366
khtml/khtmlview.cpp 28dbac3
khtml/rendering/render_form.cpp c15247a
khtml/rendering/render_table.cpp 5b07714
khtml/xpath/util.cpp 079008d

Diff: <a href="http://git.reviewboard.kde.org/r/102794/diff/diff" title="http://git.reviewboard.kde.org/r/102794/diff/diff">http://git.reviewboard.kde.org/r/102794/diff/diff</a>

Testing
no regressions in kdelibs tests.

Thanks,

Jaime Torres Amate

Comments

Re: Review Request: Avoid possible null pointer dereferences in

By Jaime Torres Amate at 10/07/2011 - 12:25

(Updated Oct. 7, 2011, 4:25 p.m.)

Review request for kdelibs.

Changes
Removed the trailing whitespace changes (I've always been asked to remove then before commiting).
Using a local ret variable and only one return.
If there is a isNotEmpty method, then I'll change !isEmpty() with it, otherwise I'll leave it untouched.

boolean disertation:
I agree with Christoph and Dijkstra.
!isEmpty() is read as "not is empty?" or as "is not empty?" and isEmpty()==false is read as "is empty false?" .Which one is easier? Please, do not answer.
In python it is if not isEmpty(): or if isEmpty()==False:

Description
The real changes are:
Avoid possible null pointer dereferences in khtml.
The common check
if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a) with null a. changed to
if (a) { if something(a)) do return bla(a) else blabla(a) }

As a side effect:
kate has removed a lot of tailing spaces in the edited files.
Avoid a possible crash checking the index limit before accesing the array.
Move some variables inside the #ifdef block where they are used

Diffs (updated)
khtml/css/css_valueimpl.cpp 3fb2898
khtml/ecma/kjs_window.cpp 0e7394b
khtml/html/htmltokenizer.cpp b64e83d
khtml/java/kjavaappletserver.cpp 234c6f3
khtml/khtml_part.cpp 53929fa
khtml/khtmlimage.cpp c6e6366
khtml/khtmlview.cpp 28dbac3
khtml/rendering/render_form.cpp c15247a
khtml/rendering/render_table.cpp 5b07714
khtml/xpath/util.cpp 079008d

Diff: <a href="http://git.reviewboard.kde.org/r/102794/diff/diff" title="http://git.reviewboard.kde.org/r/102794/diff/diff">http://git.reviewboard.kde.org/r/102794/diff/diff</a>

Testing
no regressions in kdelibs tests.

Thanks,

Jaime Torres Amate

Re: Review Request: Avoid possible null pointer dereferences in

By Commit Hook at 10/07/2011 - 12:47

This review has been submitted with commit 4eb2bb4b9b5fc206f88e56eceac2cca6b34d0521 by Jaime Torres to branch KDE/4.7.

- Commit Hook

On Oct. 7, 2011, 4:25 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Ruurd Pels at 10/07/2011 - 12:31

Ship it!

Ship It!

- Ruurd Pels

On Oct. 7, 2011, 4:25 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Ruurd Pels at 10/07/2011 - 11:47

Revert the whitespace changes with git. Other than that I dislike multiple exit points even in trivially short functions. It really does not hurt to create a local variable holding the result of the function and then return that as the result of a function. Another pet-peeve is negative tests, for example ... if ( !xxx->isEmpty() ) ... and would prefer conditions always yielding true, for example if ( xxx->isEmpty() == false) ) instead.

- Ruurd Pels

On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Ruurd Pels at 10/07/2011 - 10:34

Revert the whitespace changes with git. Other than that I dislike multiple exit points even in trivially short functions. It really does not hurt to create a local variable holding the result of the function and then return that as the result of a function. Another pet-peeve is negative tests, for example ... if ( !xxx->isEmpty() ) ... and would prefer conditions always yielding true, for example if ( xxx->isEmpty() == false) ) instead.

- Ruurd Pels

On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Ruurd Pels at 10/07/2011 - 11:49

Free after my fellow countryman Edsger Dijkstra: Negate considered harmful. It is easy to overlook the !-operator and negative tests require an extra mental step to be able to follow the logic. And yes it is a pet peeve.

- Ruurd

On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Thiago Macieira at 10/07/2011 - 12:30

On Friday, 7 de October de 2011 15:49:59 Ruurd Pels wrote:
That may be, but there's nothing in the style guide asking for this change.
I'd say it's optional if the author wants to do them.

Re: Review Request: Avoid possible null pointer dereferences in

By Christoph Feck at 10/07/2011 - 11:06

If you read the "!isEmpty" as "is not Empty", the first version sounds clearer, natural, and easier to understand than the second version.

- Christoph

On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:

Re: Review Request: Avoid possible null pointer dereferences in

By Thiago Macieira at 10/07/2011 - 10:21

You can configure kate not to remove trailing whitespaces in the lines you didn't touch.

You can also remove those unnecessary changes using Git.

- Thiago Macieira

On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote: