[webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 19, 2012, at 10:35 PM, David Barr wrote:

> On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov <ap at webkit.org> wrote:
>> 
>> I noticed a number of patches going in recently that add null checks, or refactor the code under the premise of "eliminating potential null dereference". What does that mean, and why are we allowing such patches to be landed?
> 
> When there are known conditions for which a value is null that we
> would otherwise dereference, check first and take an alternate path in
> the null case.
> 
>> For example, this change doesn't look nice: <http://trac.webkit.org/changeset/114678/trunk/Source/WebCore/css/CSSStyleSelector.cpp>. We should not try to confuse ourselves with unusual for loop forms, and there is no explanation at all of why these changes are needed. No regression tests either.
> 
> Style aside, it is quite clear that the one of the termination
> conditions for the first loop is selector == null.
> So the second loop ought to check selector != null before any
> dereference of selector.

If that code path is reachable, then it should be possible to construct a test, and the claim that there's no new tests because it's code cleanup only is wrong. If it is not reachable, then your argument does not apply

> 
> Regarding style, the change homogenizes the loop constructs within that method.
> 

That seems subjective. Making a bunch of these tiny changes that are not tied to an actual change in behavior or a clear-cut broader goal has some downsides:
- Makes it harder to study history
- Causes needless extra build thrash for people and buildbots
- Creates risk of accidentally introducing a bug

I don't have a strong opinion on this one, but if a bunch of these changes are landing, then either they need tests if they fix real bugs, or they should be related to some more concrete goal than just "code cleanup".

Regards,
Maciej