Re: [PATCH 01/12] regcache: Add functions suitable for regset_supply/collect.

Andreas Arnez
2014-05-27T14:21:13Z

On Tue, May 27 2014, Yao Qi wrote:

> On 05/27/2014 07:53 PM, Andreas Arnez wrote:
>> Correct. Maybe it's better to rephrase the whole comment like this:
>>
>
> Yes, that is much better.

Good -- thanks for pointing this out.

>
>> /* Mapping between register numbers and offsets in a buffer, for use
>> in the '*regset' functions below. In an array of
>> 'regcache_map_entry' each element is interpreted like follows:
>>
>> - If 'regno' is a register number: Map register 'regno' to the
>> current offset (starting with 0) and increase the current offset
>> by the register's size. Repeat this with consecutive register
>> numbers up to 'regno+count-1'.
>>
>> - If 'regno' has the special value REGCACHE_MAP_SKIP_BYTES: Add
>> 'count' to the current offset.
>
> Nit: I'd say "If 'regno' is REGCACHE_MAP_SKIP_BYTES, 'count' is the
> increased offset". This is just my suggestion, which may be worse
> than yours.

I agree with simplifying the phrase "has the special value" to "is".
For consistency I'd like to stick to the term "current offset" from the
previous bullet. Also, I intentionally avoid saying "increase", because
the current implementation allows 'count' to be negative. So I'll just
shorten to:

If 'regno' is REGCACHE_MAP_SKIP_BYTES: Add 'count' to the current
offset.

>
>>
>> - If count=0: End of the map. */
>>
>>> >
>>>> >> +/* Transfer a set of registers (as described by REGSET) between
>>>> >> + REGCACHE and BUF. If REGNUM == -1, transfer all registers
>>>> >> + belonging to the regset, otherwise just the register numbered
>>>> >> + REGNUM. The REGSET's 'descr' field must point to an array of
>>>> >> + 'struct regcache_map_entry'.
>>> >
>>> > IWBN to update the comments to 'descr' field, and go a step further,
>>> > rename field 'descr'.
>> With the new name being something like 'map' or 'regmap', I guess? If
>> that's what you mean, I tend to agree, and I could provide a separate
>> patch for that.
>>
>
> 'regmap' sounds good to me. I don't have other comments.

OK, I'll post a separate patch for that.