On Friday 25 June 2010 20:05:21 Eli Zaretskii wrote:

> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Fri, 25 Jun 2010 12:32:55 +0400
> > 
> > The attached patch makes a few changes in MI memory commands, with the
> > goal of making it easy for frontend to just display memory view, even
> > around the boundaries of accessible memory.
> 
> Thanks.  I have a few comments about the documentation part of the
> patch.
> 
> > +@item @var{count}
> > +The number of bytes to read. This should be an integer literal.
>                               ^^
> Two spaces here, please.
> 
> > +The commands attempts to read memory at the specified address.
>    ^^^^^^^^^^^^
> "This command"
> 
> > +                                                                When a
> > +memory map is available
> 
> A cross-reference here to where memory maps are described would be
> useful.
> 
> > +                        regions marked as unreadable in the memory
> > +map will be skipped. In addition, when @value{GDBN} encouters an error
>                       ^^
> Two spaces.
> 
> > +reading a memory range, it will attempt to find readable subrange at
>                                                   ^^^^^^^^^^^^^^^^^^
> "a readable subrange"
> 
> > +the beginning and range. Essentially, this command will make
>                     ^^    ^^
> Something ("end of the"?) is missing here.  And two spaces between
> sentences.
> 
> I'm not sure I completely understand this part:
> 
>   In addition, when @value{GDBN} encouters an error reading a memory
>   range, it will attempt to find readable subrange [...]
> 
> What if there are two or more non-contiguous sub-ranges at the
> beginning -- will GDB find and read both of them?  More generally,
> what happens if the specified range has several disjoint portions that
> are not readable?
> 
> I'm asking because your description seems to imply that we only look
> for the first readable address after start and the last readable
> address before the end of the specified region.  

Yes, that's correct. I've adjusted the wording.




> 
> If my understanding is correct, then the following sentence
> 
> > +                               Essentially, this command will make
> > +reasonable effort to return all readable memory content within the
> > +requested range.
> 
> is at least inaccurate, if not misleading ("all readable memory within
> the range").
> 
> > +The output of the command has a result record named @samp{memory},
>                              ^^^^^^^^^^^^^^^^^^^
> Perhaps "is a result record"?  

Nope. "result record" is actually a nonterminal in MI grammar, and output
of a command may have result records but is never a result record itself.

> And what is the importance of naming
> this record `memory'? why is the name important here?

Because for frontend to access a result record in a command output, it
has to know its name. 

> > +@item contents
> > +The contents of the memory block, as hex-encoded string of bytes.
> 
> But your example shows this:
> 
> > +              contents="01000000020000000300"@}]
> 
> This doesn't look like a ``string of bytes'' to me.  Or maybe I don't
> understand what you meant by that?

It seems very much like a hex-encoded string of bytes. Maybe, "hex-encoded
sequence of bytes" will work better?

> 
> > +@item @var{contents}
> > +The hex-encoded bytes to write.  The size of this parameter determines
> > +how many bytes should be written.^^^^^^^^
> 
> You probably meant "the value", not "the size".

Actually, "the size". A parameter is a string, a string has a size, and the size
of that string determines how many bytes we are writing.

What about the attached revision?

Thanks,

-- 
Vladimir Prus
CodeSourcery
vladimir@codesourcery.com
(650) 331-3385 x722