On 20 April 2012 08:18, Alexey Proskuryakov a...@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?
Sat, Apr 21, 2012 at 8:13 AM, John Yani van...@gmail.com wrote:
2316if (selector-relation() != CSSSelector::SubSelector)
2317break;
2318selector = selector-tagHistory();
2319};
Now selector is null and we are trying to call tagHistory():
Given Antti's response, I'm even less convinced that these coverity related
fixes are overall improvement (I have no doubt and am grateful that those
working on this effort is doing so on good intentions).
My suggestion is to require tests for any changes of this nature (perhaps
except
On Apr 21, 2012, at 9:45 AM, Antti Koivisto wrote:
Sat, Apr 21, 2012 at 8:13 AM, John Yani van...@gmail.com wrote:
2316if (selector-relation() != CSSSelector::SubSelector)
2317break;
2318selector = selector-tagHistory();
2319};
Now selector
On Sat, Apr 21, 2012 at 9:45 AM, Antti Koivisto koivi...@iki.fi wrote:
There is generally too much pointless drive-by refactoring going on in the
project. I think we should take harder line against these No new test /
code cleanup only type patches to reduce noise level.
+1 to this.
On Apr 19, 2012, at 10:35 PM, David Barr wrote:
On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov a...@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
I think this all started with a lot of effort put into fixing an issue
reported by a user where they said the most popular online forum in
Malaysia is broken. Then folks had to do a lot of builds (bisecting) to
track down where the problem was introduced. Then they had to figure out
what had
On Thu, Apr 19, 2012 at 10:35 PM, David Barr davidb...@google.com wrote:
Regarding style, the change homogenizes the loop constructs within that
method.
I don't think that's necessary an improvement. e.g. we don't have a style
guide that mandates it.
I completely agree with Maciej here that
On Thu, Apr 19, 2012 at 11:36 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Thu, Apr 19, 2012 at 10:35 PM, David Barr davidb...@google.com wrote:
Regarding style, the change homogenizes the loop constructs within that
method.
I don't think that's necessary an improvement. e.g. we don't have
Changing the two loops to use the same form improves readability
because it makes it clear that the form of iteration is the same
between the two loops. This is a very common C pattern when dealing
with lists where the behavior changes after an element is encountered.
The pattern is used instead
On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
macpher...@chromium.orgwrote:
Tests are a good thing, but they are not the only thing. Consider the
state-space of a large piece of software like webkit - it is
essentially infinite. You can’t test every case and code path to
ensure
On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa rn...@webkit.org wrote:
Is the code reachable? It's quite possible that the code is unreachable and
therefore there is no way to hit that crash. Without a test, we can't answer
that question.
That is not rationally true. A test case can show that
On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
macpher...@chromium.org wrote:
What do we hope to achieve by adding a test when fixing a bug? To
prevent a bug from being reintroduced into the codebase at a later
date. This is a reasonable goal, so let’s remember that the goal is to
prevent
+1 to the idea that we should try to add a test for each change. That
being said, as rniwa mentioned, it is sometimes difficult to make a
test because
(A) we do not come up with a test case
(B) we know that the code is unreachable (e.g.
https://bugs.webkit.org/show_bug.cgi?id=84377)
What is the
On Apr 20, 2012, at 11:40 AM, Benjamin Poulain benja...@webkit.org wrote:
On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
macpher...@chromium.org wrote:
What do we hope to achieve by adding a test when fixing a bug? To
prevent a bug from being reintroduced into the codebase at a later
On Fri, Apr 20, 2012 at 3:06 PM, Filip Pizlo fpi...@apple.com wrote:
Someone in this thread said something about code readability. So let's
consider that. If I see code like:
if (!var) thingy();
Then I will be under the impression that var might sometimes be zero and
that thingy() might
On Apr 19, 2012, at 11:11 PM, David Levin wrote:
I think this all started with a lot of effort put into fixing an issue
reported by a user where they said the most popular online forum in Malaysia
is broken. Then folks had to do a lot of builds (bisecting) to track down
where the problem
On Fri, Apr 20, 2012 at 1:39 PM, Maciej Stachowiak m...@apple.com wrote:
On Apr 19, 2012, at 11:11 PM, David Levin wrote:
I think this all started with a lot of effort put into fixing an issue
reported by a user where they said the most popular online forum in
Malaysia is broken. Then folks
Hi Luke,
I feel like you've been put on the defensive a bit here, which is unfortunate.
I do agree with the value of readable code and hackability is one of the core
values of the WebKit project. Code should indeed be meaningful to programmers.
One thing to keep in mind though is that, by
On Apr 20, 2012, at 12:05 PM, Kentaro Hara wrote:
+1 to the idea that we should try to add a test for each change. That
being said, as rniwa mentioned, it is sometimes difficult to make a
test because
(A) we do not come up with a test case
(B) we know that the code is unreachable (e.g.
If we had a static analyzer that ran automatically as part of the WebKit
development process, and a shared goal to get its complaints down to 0,
then it might be reasonable to skip creating tests for issues that it
diagnoses. But that doesn't seem to be the situation here.
If we ran a static
I completely agree with Maciej here that if this is a reachable code, then
the patch author should put a reasonable effort into creating a test case. And
most importantly, these changes are clearly not code cleanup.
I'm disagreeing here. (as far as NULL checks go).
Unless there's a
I completely agree with Maciej here that if this is a reachable code, then
the patch author should put a reasonable effort into creating a test case. And
most importantly, these changes are clearly not code cleanup.
I'm disagreeing here. (as far as NULL checks go).
Unless there's a
If this issue was spotted by a static analyzer, it would be good to
mention that in the ChangeLog too.
We (the static analysis loons ;) usually try to do that. (The log will
contain CID= to identify the Coverity ID, and most of the time a
[Coverity] tag).
The bug that David mentioned
On Apr 20, 2012, at 1:48 PM, Rachel Blum wrote:
I completely agree with Maciej here that if this is a reachable code, then
the patch author should put a reasonable effort into creating a test case.
And most importantly, these changes are clearly not code cleanup.
I'm disagreeing here.
On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum gr...@chromium.org wrote:
Unless there's a demonstrable reason that you _need_ a value uninitialized,
why is the burden of proof on the person doing cleanup? Yes, at the point
the code was written, it's well possible that the author was aware that
On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum gr...@chromium.org wrote:
I completely agree with Maciej here that if this is a reachable code, then
the patch author should put a reasonable effort into creating a test case.
And
most importantly, these changes are clearly not code cleanup.
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?
For example, this change doesn't look nice:
On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov a...@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
29 matches
Mail list logo
