Hi Michael, some very brief feedback:
1. MD4, MD5, and SHA1 are now implemented in wine's advapi32. Please use these instead of OpenSSL's. 2. The regression tests should be written so they don't fail if OpenSSL isn't available. 3. Get rid of magic numbers: What do the values 0x36 and 0x5c mean in copy_hmac_info? What is the length 64?
Finally, some stylistic nits: 4. Please break long lines. 5. In some of the files you have the incorrect filename in the comments, e.g. dlls/rsabase/handle.c, dlls/rsabse/handle.h 6. Your indentation in some places is inconsistent; you mix spaces and tabs, and that makes it hard for me to guess the appropriate tab size.
__________________________________ Do you Yahoo!? Yahoo! Mail Address AutoComplete - You start. We finish. http://promotions.yahoo.com/new_mail
Hello Juan, thanks for your comments!
- MD4, MD5, and SHA1 are now implemented in wine's
advapi32. Please use these instead of OpenSSL's.
Yes, that makes sense. Would it be possible to move the SHA1, MD4 and MD5 related code to some other directory in the wine tree and link to it statically from both advapi32 and rsaenh? Is the misc directory meant for this purpose?
- The regression tests should be written so they
don't fail if OpenSSL isn't available.
Yes, the regression tests definitely need some code cleanup.
- Get rid of magic numbers: What do the values 0x36
and 0x5c mean in copy_hmac_info? What is the length 64?
Those are actually part of the HMAC algorithm and don't have meaningful names. I will add references to an HMAC paper in a comment. Do you think it makes sense to call them HMAC_DEFAULT_36 and HMAC_DEFAUL_5C or something?
Finally, some stylistic nits: 4. Please break long lines.
Is there a rule for the maximum line lenght in wine? Would 128 characters be ok?
- In some of the files you have the incorrect
filename in the comments, e.g. dlls/rsabase/handle.c, dlls/rsabse/handle.h
Thanks, I'll fix those.
- Your indentation in some places is inconsistent;
you mix spaces and tabs, and that makes it hard for me to guess the appropriate tab size.
Are tabs or spaces prefered in wine? Is there a common tabwidth? How to I get vi to store tabs as white spaces?
Is it ok to post a 30kB patch to wine-devel or is it preferable to put it on the web and just post a link?
Thanks again for taking the time to review my patch.
Greetings, Michael
On Tue, Oct 26, 2004 at 10:13:59AM +0200, Michael Jung wrote:
Finally, some stylistic nits: 4. Please break long lines.
Is there a rule for the maximum line lenght in wine? Would 128 characters be ok?
There's no maximum just don't make them too long. 128 is a bit much, <100 is better.
Are tabs or spaces prefered in wine? Is there a common tabwidth? How to I get vi to store tabs as white spaces?
Tab should be 8 spaces, anything else will mess things up long term. I'd guess spaces are prefered, to avoid any problem. Also, while everybody can use whatever indentation they want (as along as they are consistent with the existing indentation when modifying files), the most common (and I guess the prefered) indentation in Wine is 4 spaces.
Is it ok to post a 30kB patch to wine-devel or is it preferable to put it on the web and just post a link?
Posting is OK (and prefered), it makes it easier to reply to it, and provide comments.
-
Dimitrie O. Paun -
Juan Lang -
Michael Jung