On 06/02/2011 05:06 AM, Julian Calaby wrote:
Roland, I've passed an eye over the entire patch set. My comments are pretty minor, but I've replied to the patches that I have concerns for. Please note that I'm not really a developer, and my comments are mostly style issues, not actual functional issues. Oh, and also, I read most of the patches out of order, so some of the comments may not make complete sense. Overall, my comments are these: 1. If you're doing typedef removals in a patch, mention it in the summary. 2. Work on your summaries, there are a few cases where I think they're a little too terse. 3. Good work! 4. Keep going =)
Thanks ! Your comments are noted. Btw, this patch train is not just the fruit of my labor, so Arend and Franky deserve this credit as well :-)
A couple of items you might want to add to your TODO list: 1. Replace the REG_[RW] macros with inline functions or equivalent because they're ugly.
Noted it down.
2. You shouldn't need to #ifdef on BIG_ENDIAN - there are a set of macros to help with working with different endian systems.
Noted that one down as well.
3. Do a typedef removal sweep over the code because typedefs are ugly =)
Agree. At the end of the patch train, all typedefs have been moved over to one file. Next step is to get rid of them.
4. I had a glance at /Documentation/CodingStyle and saw that there were some issues mentioned in there that could be fixed in your code. You might also want to play with checkpatch.
When writing new code, we adhere to checkpatch. But in these patches, a lot of (existing) code was moved around. We could have placed a separate commit after each move that fixed up checkpatch issues (not in the same commit since that blocks tools from detecting the movement of code), but at the moment the focus is on structural code cleanup (reducing number of .c and .h files, removing dead code, etc).
5. Personally, I'd avoid void pointers, but I'm not sure of the exact rules. Could someone else weigh in on this?
I am curious too, the CodingStyle doc says nothing about it.
I'm probably missing a whole lot of other, much more important, stuff, but this is what I saw.
Valuable feedback, taken into account. Thanks for investing time in this. Bye, Roland. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
