Re: [RFA v2 06/17] Record minimal symbols directly in reader.

Pedro Alves
2016-10-13T22:34:02Z

On 10/13/2016 10:10 PM, Tom Tromey wrote:
> This patch changes minimal symbol creation in two ways. First, it
> removes global variables in favor of members of minimal_symbol_reader.
> Second, it changes functions like prim_record_minimal_symbol to be
> member functions of minimal_symbol_reader.
>

Awesome, thanks.

> struct minimal_symbol *
> -prim_record_minimal_symbol_full (const char *name, int name_len, int copy_name,
> - CORE_ADDR address,
> - enum minimal_symbol_type ms_type,
> - int section,
> - struct objfile *objfile)
> +minimal_symbol_reader::record_full (const char *name, int name_len,
> + int copy_name,

Can you make this bool while at it?

> + CORE_ADDR address,
> + enum minimal_symbol_type ms_type,
> + int section)

> explicit minimal_symbol_reader (struct objfile *);
>
> @@ -73,6 +74,56 @@ class minimal_symbol_reader
>
> void install ();
>
> + /* Record a new minimal symbol. This is the "full" entry point;
> + simpler convenience entry points are also provided below.
> +
> + This returns a new minimal symbol. It is ok to modify the returned
> + minimal symbol (though generally not necessary). It is not ok,
> + though, to stash the pointer anywhere; as minimal symbols may be
> + moved after creation. The memory for the returned minimal symbol
> + is still owned by the minsyms.c code, and should not be freed.
> +
> + Arguments are:
> +
> + NAME - the symbol's name
> + NAME_LEN - the length of the name
> + COPY_NAME - if true, the minsym code must make a copy of NAME. If
> + false, then NAME must be NUL-terminated, and must have a lifetime
> + that is at least as long as OBJFILE's lifetime.
> + ADDRESS - the address of the symbol
> + MS_TYPE - the type of the symbol
> + SECTION - the symbol's section
> + appropriate obj_section for the minimal symbol. This can be NULL.

Hmm, preexisting, but, this parameter's type is actually int.
Not sure that comment makes sense as is.

> + OBJFILE - the objfile associated with the minimal symbol. */

ENOSUCHPARAMETER.

> +
> + struct minimal_symbol *record_full (const char *name,
> + int name_len,
> + int copy_name,

Use "bool" for copy_name ?

> + CORE_ADDR address,
> + enum minimal_symbol_type ms_type,
> + int section);
> +
> + /* Like record_full, but:
> + - uses strlen to compute NAME_LEN,
> + - passes COPY_NAME = 1,

"= true".

> + - and passes a default SECTION, depending on the type
> +
> + This variant does not return the new symbol. */
> +
> + void record (const char *, CORE_ADDR, enum minimal_symbol_type);

I think it may be a good idea to give names to the parameters.

> +
> + /* Like record_full, but:
> + - uses strlen to compute NAME_LEN,
> + - passes COPY_NAME = 1. */

= true.

> +
> + struct minimal_symbol *record_with_info (const char *name,
> + CORE_ADDR address,
> + enum minimal_symbol_type ms_type,
> + int section)
> + {
> + return record_full (name, strlen (name), 1, address, ms_type, section);

s/1/true/.

LGTM with those changes.

Thanks,
Pedro Alves