Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Sat, 05 May 2012 06:38:57 +0200, Sergio Durigan Junior wrote:
> On Friday, May 04 2012, Gary Benson wrote:
> > +struct probe_info
> > +  {
> > +    /* The name of the probe.  */
> > +    const char *name;
> > +
> > +    /* Nonzero if this probe must be stopped at even when
> > +       stop-on-solib-events is off.  */
> > +    int mandatory;
> 
> I don't know what others think about it, but the `mandatory' flag can be
> a bitfield, like this:
> 
>   int mandatory_p : 1;

In such case 'unsigned mandatory_p : 1' as otherwise its true value is -1.


> Also, since this is a predicate to indicate whether or not something
> happens, it's better to put the `_p' suffix.

'_p' as a predicate usually flags validity of some other field (such as
a hypothetical field 'mandatory' in this case).  I do not see the 'predicate'
need to be valid here, this is normal flag.


> > +static const struct probe_info probe_info[] =
> > +{
> > +  {"rtld_init_start", 0},
> > +  {"rtld_init_complete", 1},
> > +  {"rtld_map_start", 0},
> > +  {"rtld_reloc_complete", 1},
> > +  {"rtld_unmap_start", 0},
> > +  {"rtld_unmap_complete", 1},
> > +};
[...]
> The brackets should be indented like this:
> 
> struct foo bar[] =
>   {
>     { "bla", 0 },
>     ...
>   };

No - see GNU Coding Standards, there is an example for it.  Current GDB
codebase is not always correct in this regard.


> Also, I've been thinking about creating some predicate that would
> confirm if some probe is of certain type of not.

There is that
	gdb_assert (probe_generic->pops == &stap_probe_ops);

for this purpose.


(This is not a Gary's patch review yet.)


Thanks,
Jan