Am Sonntag, den 25.09.2005, 19:17 -0600 schrieb Vitaliy Margolen: > No kidding it's so large! You can shrink it down to probably 10% of it's > current > size.
We will see, what's left after rework. > > +/* ############################ A Separator makes the source more readable but i try to reduce that. > > + if(opt_verbose) > > + { > > + trace("-----\n"); > > + } > This is not really informative to print it at all. Accepted. > > + buffer = HeapAlloc(GetProcessHeap(), 0, size); > > + lasterror=GetLastError(); > > + if(opt_verbose) > > + { > > + trace("%p: HeapAlloc(%p, 0, 0x%lx/%ld) => (0x%lx/%ld)\n", buffer, > > + GetProcessHeap(), size, size, lasterror, lasterror); > > + > > + put_lasterror(); > > + } > > + ok(buffer != NULL, "allocating 0x%ld/%ld Bytes for Buffer => > > (0x%lx/%ld)\n", > > + size, size, lasterror, lasterror); > Will look better like this: > buffer = HeapAlloc(GetProcessHeap(), 0, size); > ok(buffer != NULL, "HeapAlloc(%ld) error=%lx\n", size, > GetLastError()); In this case, GetlastError() inside ok() works, but most times i must take care, that NtCurrentTeb()->LastErrorValue is never changed by any function called during a trace() / ok(). For this Reasons, i decided to cache the Result of GetLastError() in my code. My goal was to have the same style for every test, but I change it to be more "K.I.S.S". Since i want to test as much as possible, I test for: - the returncode, - then lasterror - and then the returned data. This is needed, because we have many bugs in all areas. Example: - Define a printer in linux. - Run wine with an app that uses winspool.drv - Delete your Linux-Printer - winspool.drv in wine will report now: * EnumPrinters => 0: correct, * GetDefaultPrinter => your deleted printer: this is wrong > Looks much better this way: > if(pGetPrinterDriverDirectoryA == NULL) return; > Wine uses this style please follow it: > if () > { > } > else > { > } Really? cvs from yesterday: grep -R "\}.*else" * | wc -l 4402 grep -R " * else" * | grep -v "\}.*else" | wc -l 10672 I can change that, but then we have: a: if (check) function; b: if (check) { function1; function2; } c: if (check) { function_true; } else { function_false; } No common Style. I learned the Style i used in my Patch from the Editor "jfe", because jfe did Autoident and other things with that Style. I try to change that in my Patches, but "not fall back" to my old Style is difficult . > > + is_result_ok(FALSE); > Is not helpfull really. Indicate what failed. Adding extra verbose traces > makes > output too big and hard to find what really failed. I will do a deep view to this.. > > +#define ENVIRONMENT_NULL 0 > > +#define ENVIRONMENT_WIN40 1 > > +#define ENVIRONMENT_W32X86 2 > > +#define ENVIRONMENT_IA64 3 > > +#define ENVIRONMENT_W32ALPHA 4 > > +#define ENVIRONMENT_W32PPC 5 > > +#define ENVIRONMENT_W32MIPS 6 > > +#define ENVIRONMENT_MAX ENVIRONMENT_W32MIPS > Please look inside include/wine/test.h and other wine test files. And don't > invent platform detection. It's already done just use it. Or don't define > something that you don't need. The Printing-Environments are defined my Microsoft. "%SystemRoot%\system32\spool\drivers" and "HKLM\System\CurrentControlSet\Control\Print\Environments" are your friends :-) PS: Our Registry-Entries for the driver is incomplete and wrong > And don't invent platform detection. It's already done just use it. Did you mean "sys_isNT" with this? When a ok() is going to fail, we must make sure that this is really a Failure on the Tested System. Example: GetDefaultPrinterW is present, but not implemented This is a failure on NT but ok on win9x. A simple if() that find an unimplemented "GetDefaultPrinterW" could just decide to return from the test_function but my way find more errors. (I knew, that this is not perfect: With "Services for Unicode" on win9x, no failure is allowed). > Most of the stuff that you added to dlls/winspool/tests/info.c is useless. I do not want to duplicate Code in the Testfile, so i concentrate the common Helper-Functions in one File. Since every Sourcefile must include the Filename as test name and "info.c" was present before, i used that. I removed many tests in different Files and the "not yet needed" Helper-Functions from "info.c" to reduce the size of the Patch. -- By By ... ... Detlef