Re: Re-implement MSVCRT *printf

[Mike McCormack]

Aneurin Price wrote:
> Comments, anyone?

Looks like you've put quite a bit of effort into that. I think it's a 
worthwhile effort, however, please consider:

* using a style more consistent with the rest of the Wine codebase
* not abusing the preprocessor in printf.h,
* not writing C code in the header in printf.h
* preventing code duplication by implementing the A functions using the 
W functions
* writing some regression tests to show your code is correct

thanks,

Mike

Re-implement MSVCRT *printf

[Aneurin Price]

Mike McCormack wrote:
> Aneurin Price wrote:
> > Comments, anyone?
>
> Looks like you've put quite a bit of effort into that. I think it's a 
> worthwhile effort, however, please consider:
>
> * using a style more consistent with the rest of the Wine codebase

Could you elaborate on this point? I'm not sure to what exactly it is 
you're referring.
> * not abusing the preprocessor in printf.h,
> * not writing C code in the header in printf.h
> Actually I don't really know why I even called it that, since it was 
> never really a header, just by analogy with scanf.h I suppose :-), but 
> see below...
> * preventing code duplication by implementing the A functions using the 
> W functions

The attached patch addresses these points, does it look any better?
I haven't tested it too rigorously yet, since I wanted some feedback 
without first spending ages checking it extensively, but it seems to be 
fine - just have to make sure I haven't forgotten some corner cases.
> * writing some regression tests to show your code is correct

I still don't really know what I might test here. Maybe try examples of 
cases where the current code does not act like Windows; there are also 
some cases where neither acts like windows (but they seem to have the 
same problems). The problem is I probably wouldn't think of cases where 
my code causes regressions (otherwise it wouldn't do so:-)), and this 
isn't really something in which you can enumerate all the different 
combinations of options (at least without infinite time and patience).
Thanks for your comments,
Aneurin Price

Re-implement MSVCRT *printf

[Jakob Eriksson]

Aneurin Price wrote:
> I still don't really know what I might test here. Maybe try examples 
> of cases where the current code does not act like Windows; there are 
> also some cases where neither acts like windows (but they seem to have 
> the same problems). The problem is I probably wouldn't think of cases 
> where my code causes regressions (otherwise it wouldn't do so:-)), and 
> this isn't really something in which you can enumerate all the 
> different combinations of options (at least without infinite time and 
> patience).

Just test a few things you can think up right away, that you expect to 
work. Also test a few things
you expect should fail. The most important thing is a few simple tests 
are commited, so others
can improve on it later, as issues come up.


regards,
Jakob Eriksson

Re-implement MSVCRT *printf

[Rein Klazes]

On Wed, 27 Oct 2004 23:13:13 +0100, you wrote:
> > * writing some regression tests to show your code is correct
>
> I still don't really know what I might test here. Maybe try examples of 
> cases where the current code does not act like Windows; there are also 
> some cases where neither acts like windows (but they seem to have the 
> same problems). The problem is I probably wouldn't think of cases where 
> my code causes regressions (otherwise it wouldn't do so:-)), and this 
> isn't really something in which you can enumerate all the different 
> combinations of options (at least without infinite time and patience).
> Thanks for your comments,

Actually there are already some printf tests in dll/msvcrt/tests/scanf.c
(despite its name). If you look at the code you see that some printf
tests ( ok( ...) ) are surrounded by a todo_wine { ... }. That means the
test is currently failing, we know that and will be noticed when the
test "suddenly" succeeds.

Just type "make test" in the dll/msvcrt/tests dir and see if your patch
has made any changes. Add more or change if you like. The documentation
in the developer guide about tests is good.

IMO this patch should not be delayed committing just because not all
possible tests are there yet. Others can help here as well, starting
with at least three printf bugs in wine's bugzilla. 

Rein.
-- 
Rein Klazes
rklazes(a)xs4all.nl