Andreas Rosenberg wrote:
+ if ( !lpcchSize ) { + SetLastError(ERROR_INVALID_PARAMETER); + return FALSE;
+ SetLastError(ERROR_MORE_DATA); + } + } + } + else + SetLastError(ERROR_REGISTRY_CORRUPT); + } + else + SetLastError(ERROR_REGISTRY_CORRUPT); return FALSE;
All these SetLastError are still potentially wrong since you don't check this error codes in your tests.
Nikolay Sivov wrote:
Andreas Rosenberg wrote:
if ( !lpcchSize ) {
SetLastError(ERROR_INVALID_PARAMETER);
return FALSE;
SetLastError(ERROR_MORE_DATA);
}
}
}
else
SetLastError(ERROR_REGISTRY_CORRUPT);
}
else
SetLastError(ERROR_REGISTRY_CORRUPT);
return FALSE;
All these SetLastError are still potentially wrong since you don't check this error codes in your tests.
Sorry, but I disagree with your opinion.
Conformance tests should verify, if the function works like documented.
There is no documentation about the last error state for this function. So it would not helpful to add a test regarding this, it would be wrong doing so.
http://msdn.microsoft.com/en-us/library/bb762280(VS.85).aspx
Read the docs regarding GetLastError:
http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx
"The error codes returned by a function are not part of the Windows API specification and can vary by operating system or device driver. For this reason, we cannot provide the complete list of error codes that can be returned by each function."
So one could leave it out or do anything that supplies a meaningfull error code. If an application should report this error to a user it may be helpfull for locating the problem.
So I don't see a reason to change this implementation.
Andreas Rosenberg wrote:
Conformance tests should verify, if the function works like documented.
Not correct. If you look at lots of conformance tests they test things that are not documented. In some cases they test things that documentation is wrong about.
Vitaliy.
Nikolay Sivov wrote:
Andreas Rosenberg wrote:
if ( !lpcchSize ) {
SetLastError(ERROR_INVALID_PARAMETER);return FALSE;SetLastError(ERROR_MORE_DATA);}}}elseSetLastError(ERROR_REGISTRY_CORRUPT);}
else
SetLastError(ERROR_REGISTRY_CORRUPT);return FALSE;
All these SetLastError are still potentially wrong since you don't check this error codes in your tests.
Sorry, but I disagree with you opinion.
A conformance test should verify if an API call works like documented.
The MSDN documentation specifies nothing regarding error codes for GetUserProfileDirectory.
http://msdn.microsoft.com/en-us/library/bb762280(VS.85).aspx
According to MSDN the error codes are not part of the API documentation:
"The error codes returned by a function are not part of the Windows API specification and can vary by operating system or device driver. For this reason, we cannot provide the complete list of error codes that can be returned by each function."
http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx
I don't consider it helpful writing a conformance test, for something that is not specified. One could simply omit the error codes, but if this call should fail an app may log/present a more meaningfull error message than without it. If the error codes should be different from Windows, this is no problem either (..may vary by operating system..).
2009/3/4 Andreas Rosenberg andreas.rosenberg@apis.de
Sorry, but I disagree with you opinion.
A conformance test should verify if an API call works like documented.
The MSDN documentation specifies nothing regarding error codes for GetUserProfileDirectory.
http://msdn.microsoft.com/en-us/library/bb762280(VS.85).aspxhttp://msdn.microsoft.com/en-us/library/bb762280%28VS.85%29.aspx
as i see it, if it specifies nothing you have two options, either you don't
Set them or you Set them and test for them. setting and not testing the errors seems wrong to me. and if you look at other wine code error codes are tested all the time, and the variation in error codes of different windows versions is one of the most ocurrent fixes to tests. you can argue if that's the correct approach or not, but that's for the comunity to achieve a consensus, which right now is testing for error codes.
Andreas Rosenberg wrote:
Nikolay Sivov wrote:
Andreas Rosenberg wrote:
if ( !lpcchSize ) {
SetLastError(ERROR_INVALID_PARAMETER);return FALSE;SetLastError(ERROR_MORE_DATA);}}}elseSetLastError(ERROR_REGISTRY_CORRUPT);}
else
SetLastError(ERROR_REGISTRY_CORRUPT);return FALSE;
All these SetLastError are still potentially wrong since you don't check this error codes in your tests.
Sorry, but I disagree with you opinion.
A conformance test should verify if an API call works like documented.
Exactly, included these error codes.
The MSDN documentation specifies nothing regarding error codes for GetUserProfileDirectory.
http://msdn.microsoft.com/en-us/library/bb762280(VS.85).aspx
According to MSDN the error codes are not part of the API documentation:
"The error codes returned by a function are not part of the Windows API specification and can vary by operating system or device driver. For this reason, we cannot provide the complete list of error codes that can be returned by each function."
That only means a loss of responsibility of error code part. Documentation specifies nothing about codes but a call sets (or not) it, and this should be verified.
http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx
I don't consider it helpful writing a conformance test, for something that is not specified. One could simply omit the error codes, but if this call should fail an app may log/present a more meaningfull error message than without it. If the error codes should be different from Windows, this is no problem either (..may vary by operating system..).
You're wrong: application could easily depend on last error code. A conformance test should verify documented behavior and investigate undocumented part of it.
I don't consider it helpful writing a conformance test, for something that is not specified. One could simply omit the error codes, but if this call should fail an app may log/present a more meaningfull error message than without it. If the error codes should be different from Windows, this is no problem either (..may vary by operating system..).
You're wrong: application could easily depend on last error code. A conformance test should verify documented behavior and investigate undocumented part of it.
http://www.winehq.org/docs/winedev-guide/testing-what
"However, undocumented behavior should not be tested for unless there is an application that relies on this behavior, and in that case the test should mention that application, or unless one can strongly expect applications to rely on this behavior, typically APIs that return the required buffer size when the buffer pointer is NULL."
What counts? What's on the web page or your statement?
Andreas Rosenberg wrote:
According to MSDN the error codes are not part of the API documentation:
"The error codes returned by a function are not part of the Windows API
But they are. MS just made their obligation to document all of their craft that much easer and that much more undefined. According to this logic "Error!" is the only message you need for anything that happened wrong.
Whenever someone implements a missing part of Wine, conformance tests that verify normal behavior really help. Checking border cases are not required unless you have a real life application that depends on such a corner case.
To apply it all here - no need to test for all possible errors in conformance tests. However you should test it yourself and use correct error codes where appropriate.
As far as your code goes:
+ r = GetUserProfileDirectoryW(htoken , NULL, NULL ); + lastError = GetLastError(); + expect(FALSE, r); + expect(lastError, ERROR_INVALID_PARAMETER); You still have to reset last error before checking it.
+ SetLastError(0xDEADAFFE); Please use more often used in Wine 0xdeadbeef.
+ expect(sizeExpected,lstrlenW(buffer)+1); Here and everywhere else - add space after coma.
+ r = GetUserProfileDirectoryW(htoken , NULL, NULL ); Either put space after on both sides or not at all ex: function( a ); or function(a);
+++ b/dlls/userenv/userenv_main.c +static WCHAR const profile_pathname[49] = { Qualifier goes before type - it's "static const WCHAR". Don't need to explicitly specify size.
+ if (lpcchSize && lpProfileDir) { + wBufLen = *lpcchSize; + bufW = HeapAlloc(GetProcessHeap(),0,wBufLen * sizeof(WCHAR)); + } + else { /* We need this to get the buffer size */ Here and everywhere else curly brackets go on their separate line (style already set for this file you have to maintain it).
+ if ( bufW ) + HeapFree(GetProcessHeap(),0,bufW); + } No need to check pointer for != NULL before freeing it.
+ res = GetUserNameW(&(userName[0]),&sizeName); Why not just "userName"?
+ lstrcatW(lpProfileDir,(LPWSTR)&(L"\")); L"" syntax for WCHAR strings is not allowed in Wine. Define it as static const WCHAR aray.
Vitaliy.
Vitaliy Margolen wrote:
"The error codes returned by a function are not part of the Windows API
But they are. MS just made their obligation to document all of their craft that much easer and that much more undefined. According to this logic "Error!" is the only message you need for anything that happened wrong.
As MS does not document the error codes, they force application developers to find out themselves, so in fact everyone relies on undocumented behavior. So Wine developers are also forced to imitate this undocumented behavior to achieve compatibility. Initially I only tried to stick to the formal rules as close as possible. I think, "black box programming" simply does not work here. But let's settle this discussion, I've got your point.
Thanks for your code critics. There are so much things to care about, and when browsing through Wine code, you can easily find enough examples, that do it different (maybe older stuff). So it's not so easy for somebody not familiar with your rules to do it right.
Andreas
-
Andreas Rosenberg -
Nikolay Sivov -
Ricardo Filipe -
Vitaliy Margolen