Re: [PATCH 4/5] Implement D primitive types in GDB

[Tom Tromey <tromey@redhat.com>]

>>>>> "Iain" == Iain Buclaw <ibuclaw@gdcproject.org> writes:

Iain> D has it's own type system separate from C.  This defines all
Iain> primitive types all found in D.

Thanks.  I think the guts of this patch are fine, just some nits around
the edges.

 
Iain> +enum d_primitive_types {

Comment before the new type.

Iain> +  d_primitive_type_cent,
Iain> +  d_primitive_type_ucent,

I don't think we need a comment for each enum constant, but it would be
nice for the ones that are "not obvious to C developers".  Subjective I
realize; but at least here I have no idea what "cent" means.

Iain> +  d_primitive_type_ifloat,
Iain> +  d_primitive_type_idouble,
Iain> +  d_primitive_type_ireal,

Or what the "i" prefix means.

Iain> +  d_primitive_type_cfloat,
Iain> +  d_primitive_type_cdouble,
Iain> +  d_primitive_type_creal,

"c" means complex maybe?

Iain> +static void
Iain> +d_language_arch_info (struct gdbarch *gdbarch,
Iain> +		      struct language_arch_info *lai)
Iain> +{

All new functions need intro comments.
It's fine if they are short; in a case like this where the new function
is the implementation of a language method, it can just mention which
method it implements.

Iain> +static void *
Iain> +build_d_types (struct gdbarch *gdbarch)
Iain> +{

Comment.

Iain> +const struct builtin_d_type *
Iain> +builtin_d_type (struct gdbarch *gdbarch)
Iain> +{

Comment.
 
Iain> +struct builtin_d_type
Iain> +{

Comment.  I think it's fine to refer readers back to the enum for the
meaning of fields here.

Iain> +# NOTE: The tests here intentionally do not require a go compiler.

Cut-and-pasto :)

Tom