[Mono-devel-list] Selective Logging patch

[Paolo Molaro]

On 06/01/03 Jerome Laban wrote:
> 	Here is an update for mini of the selective logging patch. I've also updated the man page.

I committed the utils/ changes, but see below for comments.

> +          asm      Show assembly loading messages

aload instead of asm sounds better to me, asm is too much connectted to
assembly language, while I guess someone will object if we use 'ass':-)

> Index: mono/jit/mono.c

The old jit is dead, so no need for this.

> +static void mono_gc_warn_handler(char *msg, GC_word arg);

Put a space between the function name and the parens, also in all the
calls below.

> -		g_warning ("can't resolve internal call, method is null");
> +		mono_trace_warning (MONO_TRACE_TYPE, "can't resolve internal call, method is null");

I'm not sure TRACE_TYPE is appropriate for an icall, maybe this warrants
it's own trace flag.

> --- mono/mini/driver.c	29 May 2003 09:53:39 -0000	1.18
> +++ mono/mini/driver.c	1 Jun 2003 14:44:11 -0000
> @@ -35,6 +35,7 @@
> +		} else if (strncmp (argv [i], "--log-level=", 12) == 0) {
> +			mono_trace_set_level_string (&argv [i][12]);

Use argv [i] + 12.

> typedef enum {
> 	MONO_TRACE_ASSEMBLY		= (1<<0),
> 	MONO_TRACE_TYPE			= (1<<1),
> 	MONO_TRACE_DLLIMPORT	= (1<<2),
> 	MONO_TRACE_GC			= (1<<3),
> 
> 	MONO_TRACE_ALL			=   MONO_TRACE_ASSEMBLY |
> 								MONO_TRACE_TYPE |
> 								MONO_TRACE_DLLIMPORT |
> 								MONO_TRACE_GC,

No comma at the end of the enum list.

> void 
> mono_trace_set_mask_string (char *value);

This needs to take a const char*, more below.

> static void 
> mono_trace_init (void)
> {
> 	if(level_stack == NULL) {
> 		level_stack = g_queue_new();
> 
> 		mono_trace_set_mask_string(getenv("MONO_LOG_MASK"));
> 		mono_trace_set_level_string(getenv("MONO_LOG_LEVEL"));

Add a space before the opening parens, same for the rest of the code.

> 	if(level <= current_level && mask & current_mask) {

Use parens around "mask & current_mask".

> 		MonoLogLevelEntry *entry = g_malloc(sizeof(MonoLogLevelEntry));
> 		entry->level	= current_level;
> 		entry->mask		= current_mask;
> 
>         g_queue_push_head (level_stack, (gpointer)entry);

There is some whitespace-damage here, use TABs to indent the beginning
of the code, but not to align the '=' in assignments.

> void 
> mono_trace_set_level_string (const char *value)
> {
> 	int i = 0;
> 	const char *valid_vals[] = {"error", "critical", "warning", "message", "info", "debug", NULL};
> 	const GLogLevelFlags valid_ids[] = {G_LOG_LEVEL_ERROR, G_LOG_LEVEL_CRITICAL, G_LOG_LEVEL_WARNING,
> 										G_LOG_LEVEL_MESSAGE, G_LOG_LEVEL_INFO, G_LOG_LEVEL_DEBUG };

Make the arrays static outside of the function.

> 	while(valid_vals[i]) {
> 		if(!strcmp(valid_vals[i], value)){
> 			mono_trace_set_level(valid_ids[i]);

Add a space before the opening parens, both '(' and '['.

> void 
> mono_trace_set_mask_string (char *value)

Value needs to be const char*: you're currently modifying the passed-in
string (with strtok).

> {
> 	int i;
> 	char *tok;
> 	guint32 flags = 0;
> 
> 	const char *valid_flags[] = {"asm", "type", "dll", "gc", "all", NULL};
> 	const MonoTraceMask	valid_masks[] = {MONO_TRACE_ASSEMBLY, MONO_TRACE_TYPE, MONO_TRACE_DLLIMPORT,
> 											MONO_TRACE_GC, MONO_TRACE_ALL };

Make the arrays static outside of the function.

> 	tok	= strtok (value, ",");

No need to align here.
strtok is not thread-safe, so it's better avoided, use strchr, for
example.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better