Re: [PATCH] fix PR-12417

[Tom Tromey <tromey at redhat dot com>]

Finally responding to this...

Mohsan> if (print_thread_events)
Mohsan> -    printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
Mohsan> +    printf_unfiltered (_("[New %s \"%s\"]\n"), 
Mohsan> + target_pid_to_str (ptid), thread_name (result));

Tom> This line is too long.
Tom> Also, I think the output will be weird if the thread does not have a name.

Mohsan> What I'm supposed to do about line length?

Normally one splits the line at the appropriate point, and then indents
the continuation line appropriately.  There is some discussion in the
GNU Coding Standards document, plus a plethora of examples in gdb.

In your patch the line looked like:

    printf_unfiltered (_("[New %s \"%s\"\n"), target_pid_to_str (ptid), thread_name (result));

I would write this as:

    printf_unfiltered (_("[New %s \"%s\"\n"), target_pid_to_str (ptid),
                       thread_name (result));

Mohsan> Any thread without a name is automatically assigned with the program
Mohsan> name argv[0]. So Empty string could not be a problem here.

But the new thread_name function can return "".
If this happens then the output will be strange.
It this cannot happen then thread_name ought to be changed.

I think it can happen, though, because the default to_thread_name
returns NULL.

Tom> Two thoughts come to mind for the patch.

Tom> First, perhaps a single function for emitting the thread description
Tom> would be better.  Then it could be normalized across all of gdb.
Tom> Second, it would be nice to use ui-out properly in such a function,
Tom> so that MI can see the thread name distinctly from the other bits.

Mohsan> Sorry, I didn't got your point here. Could you please elaborate it a
Mohsan> little more, as I am new to GDB.

Sure, no problem.

The patch has a number of hunks like this:

-ÂÂÂ Â printf_filtered (_("Thread %d has target id '%s'\n"),
-ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂtp->num, tmp);
+Â Â Â Â Â printf_filtered (_("Thread %d \"%s\" has target id '%s'\n"),
+ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂtp->num, thread_name (tp), tmp);

Given the fact that an empty result from thread_name will result in
strange-looking output, it would probably be better to normalize the
thread-name-printing parts into a single function.

Second, the patch says:

-Â ui_out_text (uiout, " (");
+Â ui_out_text (uiout, " \"");
+Â ui_out_text (uiout, thread_name (tp));
+Â ui_out_text (uiout, "\" (");

This means that the thread name is just dropped from the MI output.
However, since care is already taken here to use ui_out, it is
preferable to emit the thread name in an MI-readable form.  This can be
done with a call to ui_out_field_string.

Tom