Re: [PATCHv5 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Pedro Alves <palves@redhat.com>]

On 01/02/2018 03:31 PM, Andrew Burgess wrote:
> When revising this patch Simon talked about using DEF_ENUM_FLAGS_TYPE
> for the 'track_type', but that doesn't seem to work for enums nested
> within a class (as he pointed out).
> 
> I'm happy to move the enum innermost_block_tracker::track_type out of
> the class, and make it global (with a more descriptive name I guess)
> if that's what people would prefer, then I could use
> DEF_ENUM_FLAGS_TYPE.

I think using DEF_ENUM_FLAGS_TYPE would be better, but I can live
with this too.

Note: I have a patch series from last year on my github that improves
DEF_ENUM_FLAGS_TYPE such that you can use it within a namespace, and
also with "enum class", IIRC.  Not sure it'll make the "nested
within class" case work, haven't looked at it in months:

 https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

I'll try to resurrect that series in this cycle.

> 
> Otherwise, I think all of the review comments have been addressed.

Yes, I think so.  This all looks good to me, with a couple nits:

>  public:
> +  enum track_type
> +    {
> +     for_symbols = 0x1,
> +     for_registers = 0x2
> +    };
> +

This new enum needs documentation.  Also each enumerator.
What do they mean?  Can one combine them?  Etc.

IMO, innermost_block_tracker's method description comments
would be better on the declarations than the definitions,
so that one can read the class in the header and better
understand the API by reading the public/client interface,
but I won't insist on that.

Thanks much for following through with this, and thanks
for the patience.

Pedro Alves