Re: [PATCH] remote.c: Make packet_check_result return a structure

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello,

Nice change! Just a couple of small nits below, if you agree with them.

Regardless of whether you make the changes I suggest:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>


Alexandra Hájková <ahajkova@redhat.com> writes:

> @@ -9705,8 +9751,9 @@ remote_target::remote_send_printf (const char *format, ...)
>  
>    rs->buf[0] = '\0';
>    getpkt (&rs->buf);
> +  packet_result pkt_status = packet_check_result (rs->buf);

IMHO it's a bit confusing to have a variable of type packet_result named
pkt_status. I'd suggest changing to pkt_result.

>  
> -  return packet_check_result (rs->buf);
> +  return pkt_status.status ();
>  }
>  
>  /* Flash writing can take quite some time.  We'll set
> @@ -9718,7 +9765,7 @@ void
>  remote_target::flash_erase (ULONGEST address, LONGEST length)
>  {
>    int addr_size = gdbarch_addr_bit (current_inferior ()->arch ()) / 8;
> -  enum packet_result ret;
> +  enum packet_status ret;
>    scoped_restore restore_timeout
>      = make_scoped_restore (&remote_timeout, remote_flash_timeout);
>  
> @@ -11308,7 +11355,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
>    if (target_has_execution ()
>        && m_features.packet_support (PACKET_qCRC) != PACKET_DISABLE)
>      {
> -      enum packet_result result;
> +      enum packet_status result;

Similar comment here. A variable of type packet_status is named
result. The upside of preserving the current name is that the diff is
smaller, but I think the clearer code is worth having a bigger diff.

Same comment for the following hunks of the patch:

>  
>        /* Make sure the remote is pointing at the right process.  */
>        set_general_process ();
> @@ -12210,7 +12257,7 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_result result;
> +      enum packet_status result;
>  
>        strcpy (p, "qGetTLSAddr:");
>        p += strlen (p);
> @@ -12256,7 +12303,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
>        struct remote_state *rs = get_remote_state ();
>        char *p = rs->buf.data ();
>        char *endp = p + get_remote_packet_size ();
> -      enum packet_result result;
> +      enum packet_status result;
>  
>        strcpy (p, "qGetTIBAddr:");
>        p += strlen (p);
> @@ -13821,7 +13868,7 @@ remote_target::get_trace_status (struct trace_status *ts)
>  {
>    /* Initialize it just to avoid a GCC false warning.  */
>    char *p = NULL;
> -  enum packet_result result;
> +  enum packet_status result;
>    struct remote_state *rs = get_remote_state ();
>  
>    if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -14201,7 +14248,7 @@ remote_target::set_trace_buffer_size (LONGEST val)
>        struct remote_state *rs = get_remote_state ();
>        char *buf = rs->buf.data ();
>        char *endbuf = buf + get_remote_packet_size ();
> -      enum packet_result result;
> +      enum packet_status result;
>  
>        gdb_assert (val >= 0 || val == -1);
>        buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -15527,9 +15574,10 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>  
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
> +  packet_result pkt_status = packet_check_result (rs->buf);
>  
>    /* Verify if the request was successful.  */
> -  return packet_check_result (rs->buf.data ()) == PACKET_OK;
> +  return pkt_status.status () == PACKET_OK;
>  }
>  
>  /* Return true if remote target T is non-stop.  */


-- 
Thiago