On Mar 7, 2011, at 12:43 AM, Pierre Schweitzer wrote:
Hi,
this commits just shows you didn't understand how that function works. If you had, you wouldn't have said: "- Implement parameters checking in FsRtlIsNameInExpressionPrivate." It's already done.
Enlighten me please, where? Is it crypted in the last return statement? If so, how many of you would decipher, it should return TRUE if both Name and Expression are empty, and false if any of them is not empty? And how many of you would break this in an attempt to improve the algorithm and thus break that nice 119 chars return statement?
"- Add two shortcuts for common wildcard invocations to make the function faster." If you look at the algorithm, it will take at maximum as much loop iterations as name length.
Yeah, but not making those loop iterations is even better for, probably, 90% of function invocations. Also I will be honest: I'm not sure that loop is correct but I'm not sure it's incorrect either ("a joke about Schroedinger cat could be funny and not funny at the same moment of time"). Constants updates to the code making it less and less readable only add up to my concerns, with latest pool overruns too. And in contrary, at the very least I am sure those parts I committed are perfectly working code.
"- Second (main part of the function) is still under review." Thanks for discussing the issue with the original author of the algorithm, especially since you don't understand it. Furthermore, if you're not happy with said algorithm, you were asked to review it more than a year ago, and for at least 6 months. Thing that you NEVER did.
I review my own code too, what's the matter? Also, if it looked fine for me a year ago, it doesn't mean it's perfect and it should never be reviewed again. For your information, I review even the most perfectly working code in the kernel, and sometimes that's rewarded by finding nice typos leading to unpredicted results in a not very often executed branch of a code.
So, now you discovered what my mail address is, contact me before committing anything wrong/useless to current code. And as you said less than a week ago, focus on fixing things that don't work, instead of rewriting working code.
Somehow I thought that 7+ years of hacking this kernel (starting from revision 4578, and 522 kernel commits over time) give me some idea of what is correct, what is useful and what does not work.
And as someone else also said once: 1:1 isn't the solution everywhere. Or if you think so, start dropping arwinss.
Yeah, I don't disagree with my own words, however it was the case when it's a good addition to the code. Maxim Shatskih even outlined there is a bug in the original Windows implementation of that function and suggested (no, not to us, suggested to someone in some forum thread) to use something better "like grep source code".
Of course, don't take that personal, I like everyone ;-).
If I would take anything personal, I wouldn't survive in this project for that long :D
WBR, Aleksey.