Mark Brown wrote:
On Tue, Sep 13, 2011 at 12:59:35PM -0500, Timur Tabi wrote:
- iface = 0;
Random stylistic changes like this make review harder, especially when they're not mentioned in the changelog.
This is not a random stylistic change. My patch changes the way iface is calculated. Therefore, removing an unnecessary pre-initialization of iface is both obvious and expected.
The "iface = 0" line is used in the original code because the driver did not have a "default" case for the "switch" statement. Since I added a default case, there was no need to pre-initialize iface anymore.
If I had left that line in the driver, someone would have complained that I'm pre-initializing iface unnecessarily.
In this case it's not clear from the diff alone that there are no other places where iface gets written to
Ok, so that means that you need to look at the original code in order to understand the change completely. What's wrong with that? I just don't see how this is a valid criticism for any patch.
An important aspect of review is checking that the change being made actually do what they're supposed to do, unexpected additional stuff is a red flag - you have to work out if it's something the sender meant to include, what it's supposed to do and if it's a good idea.
Again, I just don't understand this complaint of my patch. I've carefully read your email, and I re-examined my patch and the original code, and I still don't think that there's anything wrong with the patch.