[PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Simon Marchi]

gdb/ChangeLog
2014-01-16  Simon Marchi  <simon.marchi@ericsson.com>

	* gdbarch.sh (gdbarch_address_class_name_to_type_flags): Add
	comments.
	* gdbarch.h: Regenerate.

diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 6b59371..20662b3 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -694,6 +694,13 @@ typedef const char * (gdbarch_address_class_type_flags_to_name_ftype) (struct gd
 extern const char * gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags);
 extern void set_gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_to_name_ftype *address_class_type_flags_to_name);

+/* Return the appropriate type_flags for the supplied address class.
+   This function should return 1 if the address class was recognized and
+   type_flags was set, zero otherwise.
+   No assumption should be made about the initial value of *type_flags_ptr,
+   which means that if it returns 1, the function should write it, even if
+   no flags are set. */
+
 extern int gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch);

 typedef int (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 9c1229e..6aa6d2c 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -628,6 +628,13 @@ v:int:cannot_step_breakpoint:::0:0::0
 v:int:have_nonsteppable_watchpoint:::0:0::0
 F:int:address_class_type_flags:int byte_size, int dwarf2_addr_class:byte_size, dwarf2_addr_class
 M:const char *:address_class_type_flags_to_name:int type_flags:type_flags
+
+# Return the appropriate type_flags for the supplied address class.
+# This function should return 1 if the address class was recognized and
+# type_flags was set, zero otherwise.
+# No assumption should be made about the initial value of *type_flags_ptr,
+# which means that if it returns 1, the function should write it, even if
+# no flags are set.
 M:int:address_class_name_to_type_flags:const char *name, int *type_flags_ptr:name, type_flags_ptr
 # Is a register in a group
 m:int:register_reggroup_p:int regnum, struct reggroup *reggroup:regnum, reggroup::default_register_reggroup_p::0

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Pedro Alves]

On 01/16/2014 06:47 PM, Simon Marchi wrote:
> +/* Return the appropriate type_flags for the supplied address class.
> +   This function should return 1 if the address class was recognized and
> +   type_flags was set, zero otherwise.

Say true/false instead of 1/zero.

> +   No assumption should be made about the initial value of *type_flags_ptr,
> +   which means that if it returns 1, the function should write it, even if
> +   no flags are set. */

This makes me a little confused.  This is a mapping/conversion function:

   class name -> type flags

I'd expect the function to recognize the name, and return a valid flag
(thus return true), or not recognize the name, and return false.

What would "even if no flags are set" mean?  What's the use case for that?
Recognizing a class name, but having that map to no flags?  As in,
ignoring the class name?  Is that useful?

-- 
Pedro Alves

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Sergio Durigan Junior]

On Thursday, January 16 2014, Pedro Alves wrote:

> On 01/16/2014 06:47 PM, Simon Marchi wrote:
>> +/* Return the appropriate type_flags for the supplied address class.
>> +   This function should return 1 if the address class was recognized and
>> +   type_flags was set, zero otherwise.
>
> Say true/false instead of 1/zero.

Sorry, but don't you think this is too nitpicking?  And is also the
first time I remember seeing such requirement.  I myself use "1/zero"
all the time, and I don't think this is an issue at all.

But I don't want to be meta-nitpicking, of course.

-- 
Sergio

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Pedro Alves]

On 01/16/2014 07:21 PM, Sergio Durigan Junior wrote:
> On Thursday, January 16 2014, Pedro Alves wrote:
> 
>> On 01/16/2014 06:47 PM, Simon Marchi wrote:
>>> +/* Return the appropriate type_flags for the supplied address class.
>>> +   This function should return 1 if the address class was recognized and
>>> +   type_flags was set, zero otherwise.
>>
>> Say true/false instead of 1/zero.
> 
> Sorry, but don't you think this is too nitpicking?  And is also the
> first time I remember seeing such requirement.  I myself use "1/zero"
> all the time, and I don't think this is an issue at all.

It's really GDB's style throughout.  It's not a big issue, and
I'd really let it go if I didn't have any other comments.  But since
I was making other comments, I took the opportunity to point that
out.  Really not to be picky at all, but to take the chance
to educate on GDB's style.

> But I don't want to be meta-nitpicking, of course.

Well, you sort of are.  ;-)

-- 
Pedro Alves

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Simon Marchi]

On 14-01-16 02:11 PM, Pedro Alves wrote:
> On 01/16/2014 06:47 PM, Simon Marchi wrote:
>> +/* Return the appropriate type_flags for the supplied address class.
>> +   This function should return 1 if the address class was recognized and
>> +   type_flags was set, zero otherwise.
> 
> Say true/false instead of 1/zero.
> 
>> +   No assumption should be made about the initial value of *type_flags_ptr,
>> +   which means that if it returns 1, the function should write it, even if
>> +   no flags are set. */
> 
> This makes me a little confused.  This is a mapping/conversion function:
> 
>    class name -> type flags
> 
> I'd expect the function to recognize the name, and return a valid flag
> (thus return true), or not recognize the name, and return false.
> 
> What would "even if no flags are set" mean?  What's the use case for that?
> Recognizing a class name, but having that map to no flags?  As in,
> ignoring the class name?  Is that useful?

In our case, address classes are used for multiple memory spaces support. For completeness sake, I wanted to allow the user to specify that a variable is in the default address space, for those who like to be explicit. An address in the default space would cause no flag to be set. So yeah, this is a little far fetched and arguable, so probably doesn't need to be mentioned here.

The part "No assumption should be made about the initial value of *type_flags_ptr" is still important I think, so that people know that you should not just OR your flags, but overwrite the whole content.

Simon

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Simon Marchi]

On 14-01-16 02:26 PM, Pedro Alves wrote:
> On 01/16/2014 07:21 PM, Sergio Durigan Junior wrote:
>> On Thursday, January 16 2014, Pedro Alves wrote:
>>
>>> On 01/16/2014 06:47 PM, Simon Marchi wrote:
>>>> +/* Return the appropriate type_flags for the supplied address class.
>>>> +   This function should return 1 if the address class was recognized and
>>>> +   type_flags was set, zero otherwise.
>>>
>>> Say true/false instead of 1/zero.
>>
>> Sorry, but don't you think this is too nitpicking?  And is also the
>> first time I remember seeing such requirement.  I myself use "1/zero"
>> all the time, and I don't think this is an issue at all.
> 
> It's really GDB's style throughout.  It's not a big issue, and
> I'd really let it go if I didn't have any other comments.  But since
> I was making other comments, I took the opportunity to point that
> out.  Really not to be picky at all, but to take the chance
> to educate on GDB's style.
> 
>> But I don't want to be meta-nitpicking, of course.
> 
> Well, you sort of are.  ;-)
> 

I'm ok with that. Actually, I looked around to try to keep the same style, but I stumbled upon a "bad" example :P

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Sergio Durigan Junior]

On Thursday, January 16 2014, Simon Marchi wrote:

> I'm ok with that. Actually, I looked around to try to keep the same style, but I stumbled upon a "bad" example :P

$ git grep "zero otherwise" | wc -l
85
$ git grep "false otherwise" | wc -l
30

Not very hard to choose a "bad" example...

Anyway, thanks a lot for the patch.

-- 
Sergio

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Pedro Alves]

On 01/16/2014 07:44 PM, Sergio Durigan Junior wrote:
> On Thursday, January 16 2014, Simon Marchi wrote:
> 
>> I'm ok with that. Actually, I looked around to try to keep the same style, but I stumbled upon a "bad" example :P
> 
> $ git grep "zero otherwise" | wc -l
> 85
> $ git grep "false otherwise" | wc -l
> 30
> 
> Not very hard to choose a "bad" example...

Alright, you win.  :-)  Wishful thinking I guess.
I do think true/false is better when we mean a boolean.
We even had talks about using gnulib's bool before.  Kind
of moot/unnecessary if we do get to move to C++ though.

-- 
Pedro Alves

Re: [PATCH] Add comments to gdbarch_address_class_name_to_type_flags

[Tom Tromey]

Pedro> We even had talks about using gnulib's bool before.

I think about it from time to time.
It's hard to know where to start, though; and I have way too many
unfinished projects already :)

Also the gnulib one is a bit funny:

   Portability problems not fixed by Gnulib:
[...]
   * Bit-fields of type `bool' are not supported.  Portable code should
     use `unsigned int foo : 1;' rather than `bool foo : 1;'.

I suppose we could add some kind of macro for bool bitfields or
something along those lines.

There are other issues mentioned too, though I consider them more minor.

Pedro> Kind of moot/unnecessary if we do get to move to C++ though.

The spelling at least would be the same, so it would not be wasted
effort.

On the whole, if someone wants to take it up, I'd be pleased.

Tom