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

[Pedro Alves <palves@redhat.com>]

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