Renaming IS_AGGR_TYPE & co

Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Hi all, hi Mark,

I'd like to finally work on this simple project but, before starting on
it, I need some guidance on related (re)names. Indeed, I noticed that we
have got, closely related:

    IS_AGGR_TYPE
    SET_IS_AGGR_TYPE
    IS_AGGR_TYPE_CODE
    PROMOTES_TO_AGGR_TYPE
    make_aggr_type (in lex.c)

In my understanding we should rename all of them, consistently, but I'm
not sure about the names, besides MAYBE_CLASS_TYPE_P for the first and,
likely, SET_MAYBE_CLASS_TYPE_P for the second.

Suggestions?

Many thanks,
Paolo.

   

Re: Renaming IS_AGGR_TYPE & co

[Mark Mitchell]

Paolo Carlini wrote:

> I'd like to finally work on this simple project but, before starting on
> it, I need some guidance on related (re)names. Indeed, I noticed that we
> have got, closely related:
> 
>     IS_AGGR_TYPE
>     SET_IS_AGGR_TYPE
>     IS_AGGR_TYPE_CODE
>     PROMOTES_TO_AGGR_TYPE
>     make_aggr_type (in lex.c)

Thank you for volunteering to working on this.

> In my understanding we should rename all of them, consistently, but I'm
> not sure about the names, besides MAYBE_CLASS_TYPE_P for the first and,
> likely, SET_MAYBE_CLASS_TYPE_P for the second.

I think IS_AGGR_TYPE should become MAYBE_CLASS_TYPE_P.  However, 
SET_IS_AGGR_TYPE should be SET_CLASS_TYPE_P; the bit flag is only set on 
class types, and setting it corresponds to the CLASS_TYPE_P macro below. 
  (MAYBE_CLASS_TYPE_P also includes things like template type parameters 
which *might* turn out to be class types.)

IS_AGGR_TYPE_CODE should be RECORD_OR_UNION_CODE_P, I think.  The 
purpose of this macro is to check whether the *lowered* representation 
of a type is a struct or union.  That applies to class types, but it 
also applies to pointers-to-function-member types.  This macro is 
currently substantially overused in the front end.  For example:

bool
is_properly_derived_from (tree derived, tree base)
{
   if (!IS_AGGR_TYPE_CODE (TREE_CODE (derived))
       || !IS_AGGR_TYPE_CODE (TREE_CODE (base)))
     return false;

That should probably just be:

  if (!CLASS_TYPE_P (derived) || !CLASS_TYPE_P (base))
     return false;

It's harmless to let pointers-to-members make it further through this 
function, but there's no reason we should, and it's confusing.

I would just remove PROMOTES_TO_AGGR_TYPE.  Replace its only use -- in 
an assertion -- with an expansion of the macro.

I think make_aggr_type should become make_class_type.  And, you should 
only be allowed to call it with code set to RECORD_TYPE or UNION_TYPE, 
which will obviate the IS_AGGR_TYPE_CODE check there.  It should just 
become:

  t = cxx_make_type (code);
  SET_CLASS_TYPE_P (t);
  return t;

The callers calling it with other codes should just use cxx_make_type 
directly.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Mark Mitchell wrote:
> Thank you for volunteering to working on this.
You are welcome. Many thanks for your help, I'm almost done. I have only
one remaining doubt (in init.c):

/* Report an error if TYPE is not a user-defined, aggregate type.  If
   OR_ELSE is nonzero, give an error message.  */

int
is_aggr_type (tree type, int or_else)
{
  if (type == error_mark_node)
    return 0;

  if (! IS_AGGR_TYPE (type))
    {
      if (or_else)
    error ("%qT is not an aggregate type", type);
      return 0;
    }
  return 1;
}

should aggr (aggregate) just become class? And it's ok to replace
IS_AGGR_TYPE with MAYBE_CLASS_TYPE_P or should be just CLASS_TYPE_P?

Thanks again,
Paolo.

Re: Renaming IS_AGGR_TYPE & co

[Mark Mitchell]

Paolo Carlini wrote:
> Mark Mitchell wrote:
>> Thank you for volunteering to working on this.
> You are welcome. Many thanks for your help, I'm almost done. I have only
> one remaining doubt (in init.c):

> should aggr (aggregate) just become class? And it's ok to replace
> IS_AGGR_TYPE with MAYBE_CLASS_TYPE_P or should be just CLASS_TYPE_P?

In all three uses of that function, we want a class type, AFAICT.  So, I 
would change the function to be is_class_type and use CLASS_TYPE_P as 
the predicate in the test.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Hi Mark,
> In all three uses of that function, we want a class type, AFAICT.  So,
> I would change the function to be is_class_type and use CLASS_TYPE_P
> as the predicate in the test.
I tried that, and unfortunately the build fails in libstdc++-v3 with the
below. I'm pretty sure (if you want I can double check) that
build_offset_ref (in init.c) is the caller passing sometimes a
non-CLASS_TYPE_P, it asserts only TYPE_P. I was about to submit a
version just using maybe_class_type_p, but was unsure about the error
message...

*/libstdc++-v3/include/ext/numeric_traits.h:131: error: ‘typename
__gnu_cxx::__conditional_type<std::__is_integer::__value,
__gnu_cxx::__numeric_traits_integer<_Value>,
__gnu_cxx::__numeric_traits_floating<_Value> >::__type’ is not a class type
/home/paolo/Gcc/svn-dirs/trunk-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/ext/numeric_traits.h:131:
internal compiler error: Segmentation fault

Paolo.

Re: Renaming IS_AGGR_TYPE & co

[Mark Mitchell]

Paolo Carlini wrote:
> Hi Mark,
>> In all three uses of that function, we want a class type, AFAICT.  So,
>> I would change the function to be is_class_type and use CLASS_TYPE_P
>> as the predicate in the test.
> I tried that, and unfortunately the build fails in libstdc++-v3 with the
> below. I'm pretty sure (if you want I can double check) that
> build_offset_ref (in init.c) is the caller passing sometimes a
> non-CLASS_TYPE_P, it asserts only TYPE_P. I was about to submit a
> version just using maybe_class_type_p, but was unsure about the error
> message...

What is the "type" in build_offset_ref at this point?  I'm guessing it's 
a TYPENAME_TYPE.  That's a little surprising, since we check for 
dependent types right above the call to is_aggr_type.  What's the call 
stack?

If it is a TYPENAME_TYPE, the fix might be to do:

   if (TREE_CODE (type) == TYPENAME_TYPE
       || class_type_p (...))
     ...

We still shouldn't need to allow things like TEMPLATE_TYPE_PARM; those 
are *always* dependent.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Hi Mark,

and sorry if this issue is taking more time than expected...
> What is the "type" in build_offset_ref at this point?  I'm guessing
> it's a TYPENAME_TYPE.  That's a little surprising, since we check for
> dependent types right above the call to is_aggr_type.  What's the call
> stack?
You are right, in the sense that actually the TYPENAME_TYPE comes from a
different caller, finish_base_specifier (in semantics.c). The below is
is the complete call stack. Then, would it make sense to change somehow
finish_base_specifier to  allow for TYPENAME_TYPEs?

Paolo.

//////////////////

(gdb) p debug_tree(type)
 <typename_type 0x2ac751fe26c0 __type VOID
    align 8 symtab 0 alias set -1 structural equality context
<record_type 0x2ac751fe2540 __conditional_type>
    chain <type_decl 0x2ac751fe2780 __type>>
$5 = void
(gdb) bt
#0  is_class_type (type=0x2ac751fe26c0, or_else=1) at
../../trunk/gcc/cp/init.c:1434
#1  0x0000000000612056 in finish_base_specifier (base=0x2ac751fe26c0,
access=0x2ac751850f60, virtual_p=0 '\0') at
../../trunk/gcc/cp/semantics.c:2472
#2  0x000000000058828b in cp_parser_base_specifier
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:15556
#3  0x0000000000587e21 in cp_parser_base_clause (parser=0x2ac751846640)
at ../../trunk/gcc/cp/parser.c:15389
#4  0x0000000000586fed in cp_parser_class_head (parser=0x2ac751846640,
nested_name_specifier_p=0x7fff59311817 "", attributes_p=0x7fff59311818,
bases=0x7fff59311808) at ../../trunk/gcc/cp/parser.c:14781
#5  0x0000000000584c10 in cp_parser_class_specifier
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:14260
#6  0x000000000057f79f in cp_parser_type_specifier
(parser=0x2ac751846640, flags=CP_PARSER_FLAGS_OPTIONAL,
decl_specs=0x7fff59311a50, is_declaration=1 '\001',
declares_class_or_enum=0x7fff593119e8, is_cv_qualifier=0x7fff593119e7
"") at ../../trunk/gcc/cp/parser.c:10626
#7  0x000000000057c108 in cp_parser_decl_specifier_seq
(parser=0x2ac751846640, flags=CP_PARSER_FLAGS_OPTIONAL,
decl_specs=0x7fff59311a50, declares_class_or_enum=0x7fff59311aac) at
../../trunk/gcc/cp/parser.c:8195
#8  0x000000000058afe8 in cp_parser_single_declaration
(parser=0x2ac751846640, checks=0x0, member_p=0 '\0',
explicit_specialization_p=0 '\0', friend_p=0x7fff59311aff "") at
../../trunk/gcc/cp/parser.c:17066
#9  0x000000000058ab17 in cp_parser_template_declaration_after_export
(parser=0x2ac751846640, member_p=0 '\0') at
../../trunk/gcc/cp/parser.c:16979
#10 0x000000000057d62b in cp_parser_template_declaration
(parser=0x2ac751846640, member_p=0 '\0') at ../../trunk/gcc/cp/parser.c:9271
#11 0x000000000057b920 in cp_parser_declaration (parser=0x2ac751846640)
at ../../trunk/gcc/cp/parser.c:7703
#12 0x000000000057b6bc in cp_parser_declaration_seq_opt
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:7634
#13 0x0000000000580e9b in cp_parser_namespace_body
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:11685
#14 0x0000000000580e61 in cp_parser_namespace_definition
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:11664
#15 0x000000000057b9e8 in cp_parser_declaration (parser=0x2ac751846640)
at ../../trunk/gcc/cp/parser.c:7731
#16 0x000000000057b6bc in cp_parser_declaration_seq_opt
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:7634
#17 0x0000000000574260 in cp_parser_translation_unit
(parser=0x2ac751846640) at ../../trunk/gcc/cp/parser.c:2971
#18 0x000000000059233d in c_parse_file () at
../../trunk/gcc/cp/parser.c:20710
#19 0x00000000006c7a3a in c_common_parse_file (set_yydebug=<value
optimized out>) at ../../trunk/gcc/c-opts.c:1280
#20 0x000000000091afa6 in toplev_main (argc=<value optimized out>,
argv=0x0) at ../../trunk/gcc/toplev.c:959
#21 0x00002ac751c10b54 in __libc_start_main () from /lib64/libc.so.6
#22 0x0000000000403f19 in _start ()

Re: Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Some additional details...
> You are right, in the sense that actually the TYPENAME_TYPE comes from a
> different caller, finish_base_specifier (in semantics.c). The below is
> is the complete call stack. Then, would it make sense to change somehow
> finish_base_specifier to  allow for TYPENAME_TYPEs?
>   
Doing, in finish_base_specifier, something like:

-  else if (! is_aggr_type (base, 1))
+  else if (TREE_CODE (base) != TYPENAME_TYPE
+          && TREE_CODE (base) != TEMPLATE_TYPE_PARM
+          && TREE_CODE (base) != BOUND_TEMPLATE_TEMPLATE_PARM
+          && ! is_class_type (base, 1))
     result = NULL_TREE;

allows the testsuite to pass...

Paolo.

Re: Renaming IS_AGGR_TYPE & co

[Mark Mitchell]

Paolo Carlini wrote:
> Some additional details...
>> You are right, in the sense that actually the TYPENAME_TYPE comes from a
>> different caller, finish_base_specifier (in semantics.c). The below is
>> is the complete call stack. Then, would it make sense to change somehow
>> finish_base_specifier to  allow for TYPENAME_TYPEs?
>>   
> Doing, in finish_base_specifier, something like:
> 
> -  else if (! is_aggr_type (base, 1))
> +  else if (TREE_CODE (base) != TYPENAME_TYPE
> +          && TREE_CODE (base) != TEMPLATE_TYPE_PARM
> +          && TREE_CODE (base) != BOUND_TEMPLATE_TEMPLATE_PARM
> +          && ! is_class_type (base, 1))
>      result = NULL_TREE;
> 
> allows the testsuite to pass...

I think that in this case you want MAYBE_CLASS_TYPE_P.

Here, the compiler is checking to see if the type is a valid base class 
type.  If it's a template type parameter, it *might* be a class type -- 
we just don't know yet.  So, we want to accept it.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Renaming IS_AGGR_TYPE & co

[Paolo Carlini]

Hi Mark,
>> -  else if (! is_aggr_type (base, 1))
>> +  else if (TREE_CODE (base) != TYPENAME_TYPE
>> +          && TREE_CODE (base) != TEMPLATE_TYPE_PARM
>> +          && TREE_CODE (base) != BOUND_TEMPLATE_TEMPLATE_PARM
>> +          && ! is_class_type (base, 1))
>>      result = NULL_TREE;
>>
>> allows the testsuite to pass...
> I think that in this case you want MAYBE_CLASS_TYPE_P.
>
> Here, the compiler is checking to see if the type is a valid base 
> class type.  If it's a template type parameter, it *might* be a class 
> type -- we just don't know yet.  So, we want to accept it.
Ok, thanks, now I see the logic pretty clearly. Note however, that the 
current is_aggr_type (would be is_class_type), beyond MAYBE_CLASS_TYPE_P 
emits *error messages*, which currently wrongly talk about 
""aggregate"". I understand that in this case only in order to preserve 
and improve the current behavior I should open code a check of 
MAYBE_CLASS_TYPE_P and an error message talking about... what?!?... just 
"class type" would be Ok for the error message?

Paolo.

Re: Renaming IS_AGGR_TYPE & co

[Mark Mitchell]

Paolo Carlini wrote:

> current is_aggr_type (would be is_class_type), beyond MAYBE_CLASS_TYPE_P 
> emits *error messages*, which currently wrongly talk about 
> ""aggregate"". 

Good catch.

> I understand that in this case only in order to preserve 
> and improve the current behavior I should open code a check of 
> MAYBE_CLASS_TYPE_P and an error message talking about... what?!?... just 
> "class type" would be Ok for the error message?

Yes, I think something like:

   "%T is not a class type", base

would be good.  If it's not MAYBE_CLASS_TYPE_P then it's certainly not a 
class, so this is an accurate error message, and it's makes clear what 
the problem is: base classes have to be *classes*.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713