Re: mscoree: Reimplemented GetCORVersion using a helper that checks properly the return buffer; shortened a few FIXMEs (1/4)
On 11/13/06, Paul Chitescu paulc@voip.null.ro wrote:
Changelog: mscoree: Reimplemented GetCORVersion using a helper that checks properly the return buffer; shortened a few FIXMEs (1/4)
Can you please put the [x/y] at the beginning of the subject, after the module name. My mailer cuts off long subjects, so I can't tell which order I need to look at the patches.
+#include "config.h" +#include "wine/port.h" +#include "wine/library.h" +#include "wine/debug.h"
Why did you add these includes (besides debug.h)?
+/* this helper is needed very often */
This is a very useless comment. Please leave it out. Also, you say this helper function is needed often, but as it stands, it's only being used once, and it hides the real purpose of the GetCORVersion function. Copying to an out parameter is pretty standard for a lot of the Win32 API, and we don't use helper functions anywhere else in the Wine code base for this purpose because of reasons stated above, and because each function usually does something different with the parameters.
+static HRESULT copyToWBuffer(LPCWSTR str, LPWSTR pBuf, DWORD cchBuf, LPDWORD pBufLen) +{ + DWORD len; + if (!(pBuf && cchBuf)) + return E_POINTER; + len = lstrlenW(str); + if (pBufLen) + *pBufLen = len; + if (cchBuf < len) + return ERROR_INSUFFICIENT_BUFFER; + lstrcpyW(pBuf, str); + return S_OK; +}
You've totally changed the whitespace style of the file in this helper function. Please leave GetCORVersion the way it was.
On Mon, 13 Nov 2006, James Hawkins wrote:
On 11/13/06, Paul Chitescu paulc@voip.null.ro wrote:
Changelog: mscoree: Reimplemented GetCORVersion using a helper that checks properly the return buffer; shortened a few FIXMEs (1/4)
Can you please put the [x/y] at the beginning of the subject, after the module name. My mailer cuts off long subjects, so I can't tell which order I need to look at the patches.
That's a good point - I'll do so in the future.
+#include "config.h" +#include "wine/port.h" +#include "wine/library.h" +#include "wine/debug.h"
Why did you add these includes (besides debug.h)?
Some later added functions require them. This patch is really a larger patch broken into parts at Alexandre's request.
+/* this helper is needed very often */
This is a very useless comment. Please leave it out. Also, you say this helper function is needed often, but as it stands, it's only being used once, and it hides the real purpose of the GetCORVersion function. Copying to an out parameter is pretty standard for a lot of the Win32 API, and we don't use helper functions anywhere else in the Wine code base for this purpose because of reasons stated above, and because each function usually does something different with the parameters.
If you look at the API this particular version of copying to a buffer or failing with E_POINTER repeats a lot. It's just that this is the first patch in a series.
+static HRESULT copyToWBuffer(LPCWSTR str, LPWSTR pBuf, DWORD cchBuf, LPDWORD pBufLen) +{
- DWORD len;
- if (!(pBuf && cchBuf))
- return E_POINTER;
- len = lstrlenW(str);
- if (pBufLen)
- *pBufLen = len;
- if (cchBuf < len)
return ERROR_INSUFFICIENT_BUFFER;- lstrcpyW(pBuf, str);
- return S_OK;
+}
You've totally changed the whitespace style of the file in this helper function. Please leave GetCORVersion the way it was.
Sorry - my editor replaced some spaces with a TAB. No big deal, only 2 lines to change.
Paul Chitescu
On 11/13/06, Paul Chitescu paulc@voip.null.ro wrote:
On Mon, 13 Nov 2006, James Hawkins wrote:
On 11/13/06, Paul Chitescu paulc@voip.null.ro wrote:
Changelog: mscoree: Reimplemented GetCORVersion using a helper that checks properly the return buffer; shortened a few FIXMEs (1/4)
Can you please put the [x/y] at the beginning of the subject, after the module name. My mailer cuts off long subjects, so I can't tell which order I need to look at the patches.
That's a good point - I'll do so in the future.
+#include "config.h" +#include "wine/port.h" +#include "wine/library.h" +#include "wine/debug.h"
Why did you add these includes (besides debug.h)?
Some later added functions require them. This patch is really a larger patch broken into parts at Alexandre's request.
Don't add headers (functions, etc) until they're needed by the current patch.
+/* this helper is needed very often */
This is a very useless comment. Please leave it out. Also, you say this helper function is needed often, but as it stands, it's only being used once, and it hides the real purpose of the GetCORVersion function. Copying to an out parameter is pretty standard for a lot of the Win32 API, and we don't use helper functions anywhere else in the Wine code base for this purpose because of reasons stated above, and because each function usually does something different with the parameters.
If you look at the API this particular version of copying to a buffer or failing with E_POINTER repeats a lot. It's just that this is the first patch in a series.
The entire process repeats a lot throughout the entire code base. Here's the problem: the code that is currently in GetCORVersion is stub functionality, and in the real implementation, there will be more functionality in between each of these parts. Factoring out this code is basically hiding the fact that it's a hack/stub. For example,
- if (!(pBuf && cchBuf))
return E_POINTER;
This should happen before anything in the function. It's the basic requirement for proceeding with the rest of the functionality (parameter checking). The rest of the function will be different for different APIs.
+static HRESULT copyToWBuffer(LPCWSTR str, LPWSTR pBuf, DWORD cchBuf, LPDWORD pBufLen) +{
- DWORD len;
- if (!(pBuf && cchBuf))
return E_POINTER;- len = lstrlenW(str);
- if (pBufLen)
*pBufLen = len;- if (cchBuf < len)
return ERROR_INSUFFICIENT_BUFFER;- lstrcpyW(pBuf, str);
- return S_OK;
+}
You've totally changed the whitespace style of the file in this helper function. Please leave GetCORVersion the way it was.
Sorry - my editor replaced some spaces with a TAB. No big deal, only 2 lines to change.
I meant the whitespace between the lines. It's really hard to read the code when the entire function is one glob with no empty lines breaking up functionality. If you really have to pull out this function, then keep the formatting the way it was before you copied it.
-
James Hawkins -
Paul Chitescu