On March 11, 2003 05:49 pm, Jon wrote:
if (debugout) MESSAGE("%lx",*arg);
if (debugout) TMSG2("%lx",*arg);
This is just too ugly! Please don't do such code uglification to save a few bytes. Beyond that, why isn't this file using the standard TRACE macro? MESSAGEs should be very rare, and are intended for the user, as such there's no need to compile them out, ever.
On Thu, Mar 13, 2003 at 01:05:21AM -0500, Dimitrie O. Paun wrote:
On March 11, 2003 05:49 pm, Jon wrote:
if (debugout) MESSAGE("%lx",*arg);
if (debugout) TMSG2("%lx",*arg);This is just too ugly! Please don't do such code uglification to save a few bytes. Beyond that, why isn't this file using the standard TRACE macro? MESSAGEs should be very rare, and are intended for the user, as such there's no need to compile them out, ever.
It is constructing lines of debugoutput out of different pieces. How to do that with TRACE?
Ciao, Marcus
On March 13, 2003 02:13 am, Marcus Meissner wrote:
It is constructing lines of debugoutput out of different pieces. How to do that with TRACE?
Just don't put the \n at the end of the TRACE:
TRACE("you "); TRACE("can "); TRACE("do "); TRACE("this\n");
It should result in:
'you can do this'
and it should be safe from threading issues, IIRC (the output should be buffered in a thread-local buffer). (I'm too lazy to check right now, I'm hoping someone will screem bloody murder if I'm wrong :)).
Not true, as it puts the name of the function being traced.
Just don't put the \n at the end of the TRACE:
TRACE("you "); TRACE("can "); TRACE("do "); TRACE("this\n");
It should result in:
'you can do this'
and it should be safe from threading issues, IIRC (the output should be buffered in a thread-local buffer). (I'm too lazy to check right now, I'm hoping someone will screem bloody murder if I'm wrong :)).
-- Dimi.
===== Sylvain Petreolle spetreolle@users.sourceforge.net Fight Spam ! EuroCauce: http://www.euro.cauce.org/en/index.html ICQ #170597259
"Don't think you are. Know you are." Morpheus, in "Matrix".
___________________________________________________________ Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français ! Yahoo! Mail : http://fr.mail.yahoo.com
On March 13, 2003 02:29 am, Sylvain Petreolle wrote:
Not true, as it puts the name of the function being traced.
Of course, silly me. Yeah, come to think of it, the DPRINTF macro is used to build such strings, together with the TRACE_ON macro. But regardless, my original objection against the ugly TMSG stuff still stands.
But since we're at the subject, maybe we could modify the debugging stuff to allow:
TRACE("you "); TRACE("can "); TRACE("do "); TRACE("this\n");
If we detect that we're appending, we just have to skip over the class:channel:function preamble (that's trivial, the preamble ends at the first space (' ')).
This way we can eliminate the DPRINTF macro which is rarely used, and thus confusing. Alexandre?
This way we can eliminate the DPRINTF macro which is rarely used, and thus confusing. Alexandre?
Hence my suggestion to only use MESSAGE and DPRINTF inside dumping functions that can be elimated with macros. But as far as cosmetics go, i don't see how TMSG and TMS2 are any more ugly than MESSAGE. Check the code before the patch, its just as bad...
The real problem with MESSAGE is that you don't know what kind of message its contructing, trace, debug or err. so therefore you cant just get rid of it in debug.h without knowing the context its used in.
The real solution is a total re-write of the dumping code in the affected files. But im working on the low level variant stuff at the moment, so that will have to wait.
Cheers, Jon
p.s. It seems the 1st 2 patches i sent (documentation 1 & 2) havent made it through, i'll resend those tomorrow.
===== "Don't wait for the seas to part, or messiahs to come; Don't you sit around and waste this chance..." - Live
jon_p_griffiths@yahoo.com
__________________________________________________ Do you Yahoo!? Yahoo! Web Hosting - establish your business online http://webhosting.yahoo.com
On Thu, 13 Mar 2003, Jon Griffiths wrote:
Hence my suggestion to only use MESSAGE and DPRINTF inside dumping functions that can be elimated with macros. But as far as cosmetics go, i don't see how TMSG and TMS2 are any more ugly than MESSAGE. Check the code before the patch, its just as bad...
True, the original sin is the way the file was written. However, Alexandre just posted a wonderful patch which allows us to rewrite it in a sane manner.
If a rewrite is not feasible now, we'd better leave it as is, rather than do all this ugly gymnastics.
Jon Griffiths jon_p_griffiths@yahoo.com writes:
Hence my suggestion to only use MESSAGE and DPRINTF inside dumping functions that can be elimated with macros. But as far as cosmetics go, i don't see how TMSG and TMS2 are any more ugly than MESSAGE. Check the code before the patch, its just as bad...
Well at least MESSAGE accepts varargs arguments...
Maybe part of the problem is that there is confusion between DPRINTF and MESSAGE, since both do the same thing right now. MESSAGE should be used for things that are always displayed, while DPRINTF is for debug output. We could certainly have DPRINTF do nothing when trace messages are compiled out; but this will require going through all the calls to make sure we use the right macro in each case.
On 13 Mar 2003, Alexandre Julliard wrote:
output. We could certainly have DPRINTF do nothing when trace messages are compiled out; but this will require going through all the calls to make sure we use the right macro in each case.
There are 767 DPRINTFs, and 736 MESSAGEs. I think DPRINTFs should just die, they are confusing, it's not clear what they output, etc. Your patch is just perfect, as it does the obvious thing the user expects. I think we can just turn most of the DPRINTFs into TRACEs, but we need to do that manually. If we do so, we could probably also simplify the code in certain places, so a manual process is better (in fact, a search & replace is the worse combination, as we'd simply lose track of the places we need to audit).
"Dimitrie O. Paun" dimi@intelliware.ca writes:
There are 767 DPRINTFs, and 736 MESSAGEs. I think DPRINTFs should just die, they are confusing, it's not clear what they output, etc. Your patch is just perfect, as it does the obvious thing the user expects. I think we can just turn most of the DPRINTFs into TRACEs, but we need to do that manually. If we do so, we could probably also simplify the code in certain places, so a manual process is better (in fact, a search & replace is the worse combination, as we'd simply lose track of the places we need to audit).
I agree most of them can be replaced by TRACE, but not all of them. There are cases where we want to dump debug output without the standard header; relay output is the obvious example. So I think a DPRINTF that can be compiled out would still be useful.
Alexandre Julliard wrote:
"Dimitrie O. Paun" dimi@intelliware.ca writes:
There are 767 DPRINTFs, and 736 MESSAGEs. I think DPRINTFs should just die, they are confusing, it's not clear what they output, etc. Your patch is just perfect, as it does the obvious thing the user expects. I think we can just turn most of the DPRINTFs into TRACEs, but we need to do that manually. If we do so, we could probably also simplify the code in certain places, so a manual process is better (in fact, a search & replace is the worse combination, as we'd simply lose track of the places we need to audit).
I agree most of them can be replaced by TRACE, but not all of them. There are cases where we want to dump debug output without the standard header; relay output is the obvious example. So I think a DPRINTF that can be compiled out would still be useful.
Well that sounds like a janitorial project. The following is a sample. This needs Alexandre's patch applied
Change Log: Replace DPRINTF wint TRACE
Files Changed: dlls/comctl32/rebar.c
On March 13, 2003 08:52 pm, Tony Lambregts wrote:
Well that sounds like a janitorial project.
Right you are: http://www.dssd.ca/wine/Wine-Janitorial.html#dprintf
On March 13, 2003 05:58 pm, Alexandre Julliard wrote:
I agree most of them can be replaced by TRACE, but not all of them. There are cases where we want to dump debug output without the standard header; relay output is the obvious example.
Speaking of which, relay output is not really a debug channel, it certainly does not output in the expected format. Why not take it out of --debugmsg, into it's own flag --relay?
And since we're on the subject, the --debugmsg is a bit awkward. I remember way back when when we settled on it, the reason it wasn't called --debug is that in that period that used to fire up the debugger. I'd love to rename it to something nicer like --trace or --debug.
"Dimitrie O. Paun" dpaun@rogers.com writes:
Speaking of which, relay output is not really a debug channel, it certainly does not output in the expected format. Why not take it out of --debugmsg, into it's own flag --relay?
It's still a debug channel, even if the format is a bit different. +snoop and +server also have different formats, and there's no reason to use separate options for that.
And since we're on the subject, the --debugmsg is a bit awkward. I remember way back when when we settled on it, the reason it wasn't called --debug is that in that period that used to fire up the debugger. I'd love to rename it to something nicer like --trace or --debug.
As long as our command line options interfere with the ones of the app, --debugmsg is at least less likely to cause conflicts.
On March 14, 2003 12:16 pm, Alexandre Julliard wrote:
It's still a debug channel, even if the format is a bit different. +snoop and +server also have different formats, and there's no reason to use separate options for that.
OK, we can look at it this way as well. But then, why not have those things use the same format as well?
relay:trace: server:trace:
is not that bad...
As long as our command line options interfere with the ones of the app, --debugmsg is at least less likely to cause conflicts.
Indeed. But why have this ambiguity in the first place. We should take first wine options, then the program. No mixing. We already support:
wine --wine-options -- program ...
IIRC, all we have to do is disable any wine option recognition after we detect the program name, no?
"Dimitrie O. Paun" dpaun@rogers.com writes:
OK, we can look at it this way as well. But then, why not have those things use the same format as well?
relay:trace: server:trace:
is not that bad...
The current format is just fine IMO. Relay traces are already large enough, we don't need to add another 15 characters to every line.
Indeed. But why have this ambiguity in the first place. We should take first wine options, then the program. No mixing. We already support:
wine --wine-options -- program ...
IIRC, all we have to do is disable any wine option recognition after we detect the program name, no?
For wine itself yes, but you can't do that for Winelib apps.
"Dimitrie O. Paun" dpaun@rogers.com writes:
If we detect that we're appending, we just have to skip over the class:channel:function preamble (that's trivial, the preamble ends at the first space (' ')).
This way we can eliminate the DPRINTF macro which is rarely used, and thus confusing. Alexandre?
No objection. Something like this should do the trick:
Index: dlls/ntdll/debugtools.c =================================================================== RCS file: /opt/cvs-commit/wine/dlls/ntdll/debugtools.c,v retrieving revision 1.28 diff -u -r1.28 debugtools.c --- dlls/ntdll/debugtools.c 18 Feb 2003 23:29:48 -0000 1.28 +++ dlls/ntdll/debugtools.c 13 Mar 2003 18:36:05 -0000 @@ -304,12 +304,17 @@ const char *function, const char *format, va_list args ) { static const char *classes[] = { "fixme", "err", "warn", "trace" }; + struct debug_info *info = get_info(); int ret = 0;
- if (TRACE_ON(tid)) - ret = wine_dbg_printf( "%04lx:", NtCurrentTeb()->tid ); - if (cls < sizeof(classes)/sizeof(classes[0])) - ret += wine_dbg_printf( "%s:%s:%s ", classes[cls], channel + 1, function ); + /* only print header if we are at the beginning of the line */ + if (info->out_pos == info->output || info->out_pos[-1] == '\n') + { + if (TRACE_ON(tid)) + ret = wine_dbg_printf( "%04lx:", NtCurrentTeb()->tid ); + if (cls < sizeof(classes)/sizeof(classes[0])) + ret += wine_dbg_printf( "%s:%s:%s ", classes[cls], channel + 1, function ); + } if (format) ret += NTDLL_dbg_vprintf( format, args ); return ret;
On 13 Mar 2003, Alexandre Julliard wrote:
No objection. Something like this should do the trick:
Cool! Can you (pretty) please check it in? :)
-
Alexandre Julliard -
Dimitrie O. Paun -
Dimitrie O. Paun -
Jon Griffiths -
Marcus Meissner -
Sylvain Petreolle -
Tony Lambregts