Re: [PATCH 1/3] Rename some trace functions

[Pedro Alves <palves at redhat dot com>]

On 12/02/2016 06:47 PM, Simon Marchi wrote:
> On 2016-12-02 11:47, Pedro Alves wrote:
>>> Writing a patch.
> 
> I had started a patch as well, but was interrupted by a meeting, so I
> didn't get very far.  Thanks for doing it!
> 
>> Turns out I quoted the MI names, which are a bit different
>> from the RSP names.  Sigh...
>>
>> Anyway, here's what it ends up looking like.  I changed
>> the text of the "request" stop reason, because I thought that
>> might be a tiny bit more user friendly for the case of the
>> user using an MI frontend who clicks some "trace stop" button
>> on the GUI instead of running a "command".
> 
> The new message "Trace stopped on user request" looks good to me.
> 
>> +/* See tracepoint.h.  */
>> +
>> +const char *
>> +get_rsp_name (trace_stop_reason reason)
>> +{
>> +  return rsp_trace_stop_reason_names[(int) reason];
> 
> Should we prefer static_cast<int>() over (int)?

Seems unnecessarily verbose to me when just doing a simple integer
conversion.  E.g.:

 static_cast<int> (foo) + static_cast<int> (bar) + static_cast<int> (qux)

vs

 (int) foo + (int) bar + (int) qux

I don't think there's a real advantage.

> 
> It might be a good idea to check that the resulting integer is smaller
> than the array size.  If we ever add new stop reasons, we could forget
> to add array elements, so an error() here would catch it.

That's highly unlikely, given that if you're adding a new reason,
you'll need to be able to parse it from RSP, which requires
get_rsp_name.

How about just adding comments:

diff --git i/gdb/tracepoint.c w/gdb/tracepoint.c
index 4ff1d76..89d4fa6 100644
--- i/gdb/tracepoint.c
+++ w/gdb/tracepoint.c
@@ -190,6 +190,8 @@ extern void _initialize_tracepoint (void);
 
 static struct trace_status trace_status;
 
+/* RSP string representation of trace_stop_reason.  */
+
 static const char *rsp_trace_stop_reason_names[] = {
   "tunknown",
   "tnotrun",
diff --git i/gdb/tracepoint.h w/gdb/tracepoint.h
index bbb4c4b..9840cd4 100644
--- i/gdb/tracepoint.h
+++ w/gdb/tracepoint.h
@@ -70,7 +70,9 @@ struct trace_state_variable
     int builtin;
    };
 
-/* The reason why the tracing was stopped last time.  */
+/* The reason why the tracing was stopped last time.
+
+   (If you change this, remember to update get_rsp_name as well.)  */
 
 enum class trace_stop_reason
   {

Thanks,
Pedro Alves