Re: [PATCH 1/3] inf-ptrace: Do not stop memory transfers after a single word

[Simon Marchi <simon.marchi@ericsson.com>]

On 17-03-09 12:22 PM, Andreas Arnez wrote:
>> This is not a comment specifically about your patch, since that's how
>> it was already, but maybe it would be a good time to address this.  I
>> understand there's some level of overlap between the read and write
>> (like the offset/skip computation), but I don't think that handling
>> reading and writing in the same loop is very readable.  It just adds a
>> bunch of branches and makes it hard to follow.  If that code was split
>> in two functions (one for read, one for write), it would be way more
>> straightforward.
> 
> That's probably a matter of taste.  Note that we do have separate
> routines in gdbserver/linux-low.c that fulfill the equivalent function:
> linux_read_memory() and linux_write_memory().  IMO they have even worse
> readability *plus* suffer from heavy code duplication.  Maybe that's
> just me, though.

It's very possible, I just had this thought while reading the patch, I haven't actually tried.

> Another thought that crossed my mind is whether we should extract the
> whole peek/poke loop into a separate function instead of packing all the
> logic under a case statement.  So far I didn't, because I wanted to keep
> the bug fix small.

IMO it never hurt to split up big functions in smaller, more manageable parts...

> (1) The GDB C/C++ coding standards provide a dubious explanation for the
> "NULL Is Not Zero" rule
> (https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero):
> 
>   "Zero constant (0) is not interchangeable with a null pointer constant
>   (NULL) anywhere. GCC does not give a warning for such interchange."
> 
> To me this seems to imply that the language does not support the
> interchangeability.  But according to the C standard, it does:
> 
>   "An integer constant expression with the value 0, or such an
>   expression cast to type void *, is called a null pointer constant."
> 
> C++ has a similar definition and specifies boolean conversion from
> pointer types as well.  See also Stroustrup's style FAQ "should I use
> NULL or 0?": http://www.stroustrup.com/bs_faq2.html#null
> 
> So maybe we want to support non-conforming compilers?  Or is this in
> fact a GDB-specific style rule?  In either case we should adjust the
> explanation, I think.
> 

I don't the idea behind that rule.  I thought it was just for readability,
to make it clear that the variable is a pointer without having to refer to the
declaration.  Perhaps some older timers could shed some light on that :).

Simon