C++ PATCH: use identifier_p instead of naked TREE_CODE == IDENTIFIER_NODE

This patch introduces identified_p (t) in lieu of 

   TREE_CODE (t) == IDENTIFIER_NODE

in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
except that it returns a typed pointer instead of a boolean value.

There is NO change in functionality.  With this patch, I measured that
bootsrapping takes slightly less time than without, and the object size
of cc1plus decreases by more than 5K.


Bootstrapped and texted on an x86_64-suse-linux.
Applying to trunk.  Follow up patches will exploit the typed pointer.

-- Gaby

2013-03-21  Gabriel Dos Reis  <gdr@integrable-solutions.net>

	* cp-tree.h (identifier_p): New.
	* call.c: Throughout, call identifier_p insstead of direct
	comparaison of TREE_CODE against IDENTIFIER_NODE.
	* decl.c: Likewisse.
	* decl2.c: Likewise.
	* init.c: Likewise.
	* mangle.c: Likewise.
	* name-lookup.c: Likewise.
	* parser.c: Likewise.
	* pt.c: Likewise.
	* search.c: Likewise.
	* semantics.c: Likewise.
	* tree.c: Likewise.
	* typeck.c: Likewise.
	* typeck2.c: Likewise.

Gabriel Dos Reis <gdr@axiomatics.org> writes:
> in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
> except that it returns a typed pointer instead of a boolean value.

What's the point of returning a pointer when the name (and apparently,
use, judging from the patch) suggest a boolean...?

[If it's intended to be used in non-boolean contexts as well, using
"_p" for the name seems vaguely ill-considered...]

-miles

On Thu, Mar 21, 2013 at 11:25 PM, Miles Bader <miles@gnu.org> wrote:
>
> Gabriel Dos Reis <gdr@axiomatics.org> writes:
> > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
> > except that it returns a typed pointer instead of a boolean value.
>
> What's the point of returning a pointer when the name (and apparently,
> use, judging from the patch) suggest a boolean...?

consider it to be a generalized boolean, nothing out of ordinary.

In many places, we do thinks like:
  1. test that we have a identifier.
  2. immediately follow that with access to parts of the
      tree as identifiers, but check again that we really
      an identifier, etc.
      Something best written as
         if (lang_identifier *id  = identifier_p (t))
               access members of id with no more check please.

There is nothing silly about that.

-- Gaby

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
> In many places, we do thinks like:
>   1. test that we have a identifier.
>   2. immediately follow that with access to parts of the
>       tree as identifiers, but check again that we really
>       an identifier, etc.
>
> There is nothing silly about that.

Sure, it's a common and useful pattern.  I'm just saying it's silly to
call it "..._p" in that case...

-miles

On Fri, Mar 22, 2013 at 4:50 AM, Gabriel Dos Reis <gdr@axiomatics.org> wrote:
>
> This patch introduces identified_p (t) in lieu of
>
>    TREE_CODE (t) == IDENTIFIER_NODE

Generally we have macros like IDENTIFIER_P for this kind of checks.

> in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
> except that it returns a typed pointer instead of a boolean value.

Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
with kind-of dynamic_cast<identifier> (t) (in C++ terms)?  Then
naming it identifier_p is bad.  We have is-a.h now, so please try to use
a single style of C++-style casting throughout GCC.

Thanks,
Richard.

> There is NO change in functionality.  With this patch, I measured that
> bootsrapping takes slightly less time than without, and the object size
> of cc1plus decreases by more than 5K.
>
>
> Bootstrapped and texted on an x86_64-suse-linux.
> Applying to trunk.  Follow up patches will exploit the typed pointer.
>
> -- Gaby
>
> 2013-03-21  Gabriel Dos Reis  <gdr@integrable-solutions.net>
>
>         * cp-tree.h (identifier_p): New.
>         * call.c: Throughout, call identifier_p insstead of direct
>         comparaison of TREE_CODE against IDENTIFIER_NODE.
>         * decl.c: Likewisse.
>         * decl2.c: Likewise.
>         * init.c: Likewise.
>         * mangle.c: Likewise.
>         * name-lookup.c: Likewise.
>         * parser.c: Likewise.
>         * pt.c: Likewise.
>         * search.c: Likewise.
>         * semantics.c: Likewise.
>         * tree.c: Likewise.
>         * typeck.c: Likewise.
>         * typeck2.c: Likewise.
>
> Index: call.c
> ===================================================================
> --- call.c      (revision 196891)
> +++ call.c      (working copy)
> @@ -233,7 +233,7 @@
>      name = TREE_TYPE (name);
>    else if (TYPE_P (name))
>      /* OK */;
> -  else if (TREE_CODE (name) == IDENTIFIER_NODE)
> +  else if (identifier_p (name))
>      {
>        if ((MAYBE_CLASS_TYPE_P (basetype)
>            && name == constructor_name (basetype))
> @@ -3147,7 +3147,7 @@
>                      : ACONCAT ((msgstr, " ", NULL)));
>    location_t cloc = location_of (candidate->fn);
>
> -  if (TREE_CODE (candidate->fn) == IDENTIFIER_NODE)
> +  if (identifier_p (candidate->fn))
>      {
>        cloc = loc;
>        if (candidate->num_convs == 3)
> @@ -8563,8 +8563,7 @@
>       - do not have the same parameter type list as any non-template
>         non-member candidate.  */
>
> -  if (TREE_CODE (cand1->fn) == IDENTIFIER_NODE
> -      || TREE_CODE (cand2->fn) == IDENTIFIER_NODE)
> +  if (identifier_p (cand1->fn) || identifier_p (cand2->fn))
>      {
>        for (i = 0; i < len; ++i)
>         if (!same_type_p (cand1->convs[i]->type,
> @@ -8575,7 +8574,7 @@
>           if (cand1->fn == cand2->fn)
>             /* Two built-in candidates; arbitrarily pick one.  */
>             return 1;
> -         else if (TREE_CODE (cand1->fn) == IDENTIFIER_NODE)
> +         else if (identifier_p (cand1->fn))
>             /* cand1 is built-in; prefer cand2.  */
>             return -1;
>           else
> Index: cp-tree.h
> ===================================================================
> --- cp-tree.h   (revision 196891)
> +++ cp-tree.h   (working copy)
> @@ -241,6 +241,16 @@
>    tree label_value;
>  };
>
> +/* Return a typed pointer version of T if it designates a
> +   C++ front-end identifier.  */
> +inline lang_identifier*
> +identifier_p (tree t)
> +{
> +  if (TREE_CODE (t) == IDENTIFIER_NODE)
> +    return (lang_identifier*) t;
> +  return NULL;
> +}
> +
>  /* In an IDENTIFIER_NODE, nonzero if this identifier is actually a
>     keyword.  C_RID_CODE (node) is then the RID_* value of the keyword,
>     and C_RID_YYCODE is the token number wanted by Yacc.  */
> Index: decl.c
> ===================================================================
> --- decl.c      (revision 196891)
> +++ decl.c      (working copy)
> @@ -3296,7 +3296,7 @@
>         error ("%qD used without template parameters", name);
>        return error_mark_node;
>      }
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>    gcc_assert (TYPE_P (context));
>
>    if (!MAYBE_CLASS_TYPE_P (context))
> @@ -3402,7 +3402,7 @@
>      name = TYPE_IDENTIFIER (name);
>    else if (DECL_P (name))
>      name = DECL_NAME (name);
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    if (!dependent_type_p (context)
>        || currently_open_class (context))
> @@ -4781,7 +4781,7 @@
>         }
>        else
>         {
> -         gcc_assert (TREE_CODE (ce->index) == IDENTIFIER_NODE);
> +         gcc_assert (identifier_p (ce->index));
>           error ("name %qD used in a GNU-style designated "
>                  "initializer for an array", ce->index);
>         }
> @@ -7413,8 +7413,7 @@
>                           == current_class_type);
>               fns = TREE_OPERAND (fns, 1);
>             }
> -         gcc_assert (TREE_CODE (fns) == IDENTIFIER_NODE
> -                     || TREE_CODE (fns) == OVERLOAD);
> +         gcc_assert (identifier_p (fns) || TREE_CODE (fns) == OVERLOAD);
>           DECL_TEMPLATE_INFO (decl) = build_template_info (fns, args);
>
>           for (t = TYPE_ARG_TYPES (TREE_TYPE (decl)); t; t = TREE_CHAIN (t))
> @@ -7772,7 +7771,7 @@
>    tree decl;
>    tree explicit_scope;
>
> -  gcc_assert (!name || TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (!name || identifier_p (name));
>
>    /* Compute the scope in which to place the variable, but remember
>       whether or not that scope was explicitly specified by the user.   */
> @@ -8509,7 +8508,7 @@
>      {
>        if (!identifier)
>         error ("unnamed variable or field declared void");
> -      else if (TREE_CODE (identifier) == IDENTIFIER_NODE)
> +      else if (identifier_p (identifier))
>         {
>           gcc_assert (!IDENTIFIER_OPNAME_P (identifier));
>           error ("variable or field %qE declared void", identifier);
> @@ -8778,7 +8777,7 @@
>                   tree fns = TREE_OPERAND (decl, 0);
>
>                   dname = fns;
> -                 if (TREE_CODE (dname) != IDENTIFIER_NODE)
> +                 if (!identifier_p (dname))
>                     {
>                       gcc_assert (is_overloaded_fn (dname));
>                       dname = DECL_NAME (get_first_fn (dname));
> @@ -8787,7 +8786,7 @@
>                 /* Fall through.  */
>
>               case IDENTIFIER_NODE:
> -               if (TREE_CODE (decl) == IDENTIFIER_NODE)
> +               if (identifier_p (decl))
>                   dname = decl;
>
>                 if (C_IS_RESERVED_WORD (dname))
> @@ -8852,7 +8851,7 @@
>      }
>
>    if (dname
> -      && TREE_CODE (dname) == IDENTIFIER_NODE
> +      && identifier_p (dname)
>        && UDLIT_OPER_P (dname)
>        && innermost_code != cdk_function)
>      {
> @@ -8977,7 +8976,7 @@
>          common.  With no options, it is allowed.  With -Wreturn-type,
>          it is a warning.  It is only an error with -pedantic-errors.  */
>        is_main = (funcdef_flag
> -                && dname && TREE_CODE (dname) == IDENTIFIER_NODE
> +                && dname && identifier_p (dname)
>                  && MAIN_NAME_P (dname)
>                  && ctype == NULL_TREE
>                  && in_namespace == NULL_TREE
> @@ -11896,7 +11895,7 @@
>    tree context = NULL_TREE;
>    tag_scope scope;
>
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    switch (tag_code)
>      {
> @@ -12323,7 +12322,7 @@
>             bool scoped_enum_p, bool *is_new)
>  {
>    tree prevtype = NULL_TREE;
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    if (is_new)
>      *is_new = false;
> Index: decl2.c
> ===================================================================
> --- decl2.c     (revision 196891)
> +++ decl2.c     (working copy)
> @@ -1121,7 +1121,7 @@
>          second and following arguments.  Attributes like mode, format,
>          cleanup and several target specific attributes aren't late
>          just because they have an IDENTIFIER_NODE as first argument.  */
> -      if (arg == args && TREE_CODE (t) == IDENTIFIER_NODE)
> +      if (arg == args && identifier_p (t))
>         continue;
>
>        if (value_dependent_expression_p (t)
> Index: init.c
> ===================================================================
> --- init.c      (revision 196891)
> +++ init.c      (working copy)
> @@ -1416,7 +1416,7 @@
>      }
>    else
>      {
> -      if (TREE_CODE (name) == IDENTIFIER_NODE)
> +      if (identifier_p (name))
>         field = lookup_field (current_class_type, name, 1, false);
>        else
>         field = name;
> Index: mangle.c
> ===================================================================
> --- mangle.c    (revision 196891)
> +++ mangle.c    (working copy)
> @@ -1189,7 +1189,7 @@
>  {
>    MANGLE_TRACE_TREE ("unqualified-name", decl);
>
> -  if (TREE_CODE (decl) == IDENTIFIER_NODE)
> +  if (identifier_p (decl))
>      {
>        write_unqualified_id (decl);
>        return;
> @@ -2519,7 +2519,7 @@
>  static void
>  write_member_name (tree member)
>  {
> -  if (TREE_CODE (member) == IDENTIFIER_NODE)
> +  if (identifier_p (member))
>      write_unqualified_id (member);
>    else if (DECL_P (member))
>      write_unqualified_name (member);
> @@ -2697,7 +2697,7 @@
>      {
>        write_expression (TREE_OPERAND (expr, 0));
>      }
> -  else if (TREE_CODE (expr) == IDENTIFIER_NODE)
> +  else if (identifier_p (expr))
>      {
>        /* An operator name appearing as a dependent name needs to be
>          specially marked to disambiguate between a use of the operator
> Index: name-lookup.c
> ===================================================================
> --- name-lookup.c       (revision 196891)
> +++ name-lookup.c       (working copy)
> @@ -1959,7 +1959,7 @@
>    if (!name)
>      return false;
>
> -  if (TREE_CODE (name) != IDENTIFIER_NODE)
> +  if (!identifier_p (name))
>      return false;
>
>    /* These don't have names.  */
> @@ -2073,7 +2073,7 @@
>    gcc_assert (function && TREE_CODE (function) == FUNCTION_DECL);
>
>    name = DECL_NAME (function);
> -  gcc_assert (name && TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (name && identifier_p (name));
>
>    for (iter = IDENTIFIER_NAMESPACE_BINDINGS (name);
>         iter;
> @@ -2136,7 +2136,7 @@
>    tree decl;
>
>    gcc_assert (TREE_CODE (scope) == NAMESPACE_DECL);
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>    for (decl = current_binding_level->usings; decl; decl = DECL_CHAIN (decl))
>      if (USING_DECL_SCOPE (decl) == scope && DECL_NAME (decl) == name)
>        break;
> @@ -5724,7 +5724,7 @@
>                  || COMPLETE_TYPE_P (b->this_entity))))
>      b = b->level_chain;
>
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    /* Do C++ gratuitous typedefing.  */
>    if (identifier_type_value_1 (name) != type)
> Index: parser.c
> ===================================================================
> --- parser.c    (revision 196891)
> +++ parser.c    (working copy)
> @@ -1110,7 +1110,7 @@
>      case CPP_KEYWORD:
>        /* Some keywords have a value that is not an IDENTIFIER_NODE.
>          For example, `struct' is mapped to an INTEGER_CST.  */
> -      if (TREE_CODE (token->u.value) != IDENTIFIER_NODE)
> +      if (!identifier_p (token->u.value))
>         break;
>        /* else fall through */
>      case CPP_NAME:
> @@ -1259,7 +1259,7 @@
>    if (qualifying_scope && TYPE_P (qualifying_scope))
>      qualifying_scope = TYPE_MAIN_VARIANT (qualifying_scope);
>
> -  gcc_assert (TREE_CODE (unqualified_name) == IDENTIFIER_NODE
> +  gcc_assert (identifier_p (unqualified_name)
>               || TREE_CODE (unqualified_name) == BIT_NOT_EXPR
>               || TREE_CODE (unqualified_name) == TEMPLATE_ID_EXPR);
>
> @@ -2587,7 +2587,7 @@
>      {
>        if (TYPE_P (type))
>         error_at (location, "%qT is not a template", type);
> -      else if (TREE_CODE (type) == IDENTIFIER_NODE)
> +      else if (identifier_p (type))
>         {
>           if (tag_type != none_type)
>             error_at (location, "%qE is not a class template", type);
> @@ -3193,7 +3193,7 @@
>                               tree id, location_t id_location)
>  {
>    tree result;
> -  if (TREE_CODE (id) == IDENTIFIER_NODE)
> +  if (identifier_p (id))
>      {
>        result = make_typename_type (scope, id, typename_type,
>                                    /*complain=*/tf_none);
> @@ -5654,7 +5654,7 @@
>    while (true)
>      {
>        if (idk == CP_ID_KIND_UNQUALIFIED
> -         && TREE_CODE (postfix_expression) == IDENTIFIER_NODE
> +         && identifier_p (postfix_expression)
>           && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN))
>         /* It is not a Koenig lookup function call.  */
>         postfix_expression
> @@ -5741,7 +5741,7 @@
>             if (idk == CP_ID_KIND_UNQUALIFIED
>                 || idk == CP_ID_KIND_TEMPLATE_ID)
>               {
> -               if (TREE_CODE (postfix_expression) == IDENTIFIER_NODE)
> +               if (identifier_p (postfix_expression))
>                   {
>                     if (!args->is_empty ())
>                       {
> @@ -11310,7 +11310,7 @@
>        cp_id_kind idk;
>        const char *error_msg;
>
> -      if (TREE_CODE (expr) == IDENTIFIER_NODE)
> +      if (identifier_p (expr))
>         /* Lookup the name we got back from the id-expression.  */
>         expr = cp_parser_lookup_name (parser, expr,
>                                       none_type,
> @@ -12759,7 +12759,7 @@
>      }
>
>    /* Build a representation of the specialization.  */
> -  if (TREE_CODE (templ) == IDENTIFIER_NODE)
> +  if (identifier_p (templ))
>      template_id = build_min_nt_loc (next_token->location,
>                                     TEMPLATE_ID_EXPR,
>                                     templ, arguments);
> @@ -13980,7 +13980,7 @@
>           && !global_p
>           && !qualified_p
>           && TREE_CODE (type) == TYPE_DECL
> -         && TREE_CODE (DECL_NAME (type)) == IDENTIFIER_NODE)
> +         && identifier_p (DECL_NAME (type)))
>         maybe_note_name_used_in_class (DECL_NAME (type), type);
>        /* If it didn't work out, we don't have a TYPE.  */
>        if ((flags & CP_PARSER_FLAGS_OPTIONAL)
> @@ -15279,7 +15279,7 @@
>       depending on what scope we are in.  */
>    if (qscope == error_mark_node || identifier == error_mark_node)
>      ;
> -  else if (TREE_CODE (identifier) != IDENTIFIER_NODE
> +  else if (!identifier_p (identifier)
>            && TREE_CODE (identifier) != BIT_NOT_EXPR)
>      /* [namespace.udecl]
>
> @@ -16562,8 +16562,7 @@
>                   unqualified_name = error_mark_node;
>                 else if (unqualified_name
>                          && (qualifying_scope
> -                            || (TREE_CODE (unqualified_name)
> -                                != IDENTIFIER_NODE)))
> +                            || (!identifier_p (unqualified_name))))
>                   {
>                     cp_parser_error (parser, "expected unqualified-id");
>                     unqualified_name = error_mark_node;
> @@ -18226,7 +18225,7 @@
>
>    /* Check to see that it is really the name of a class.  */
>    if (TREE_CODE (decl) == TEMPLATE_ID_EXPR
> -      && TREE_CODE (TREE_OPERAND (decl, 0)) == IDENTIFIER_NODE
> +      && identifier_p (TREE_OPERAND (decl, 0))
>        && cp_lexer_next_token_is (parser->lexer, CPP_SCOPE))
>      /* Situations like this:
>
> @@ -21120,7 +21119,7 @@
>    /* By this point, the NAME should be an ordinary identifier.  If
>       the id-expression was a qualified name, the qualifying scope is
>       stored in PARSER->SCOPE at this point.  */
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    /* Perform the lookup.  */
>    if (parser->scope)
> @@ -24290,7 +24289,7 @@
>
>    node = token->u.value;
>
> -  while (node && TREE_CODE (node) == IDENTIFIER_NODE
> +  while (node && identifier_p (node)
>          && (node == ridpointers [(int) RID_IN]
>              || node == ridpointers [(int) RID_OUT]
>              || node == ridpointers [(int) RID_INOUT]
> Index: pt.c
> ===================================================================
> --- pt.c        (revision 196891)
> +++ pt.c        (working copy)
> @@ -2467,7 +2467,7 @@
>         {
>           tree fns;
>
> -         gcc_assert (TREE_CODE (declarator) == IDENTIFIER_NODE);
> +         gcc_assert (identifier_p (declarator));
>           if (ctype)
>             fns = dname;
>           else
> @@ -2528,8 +2528,7 @@
>           return decl;
>         }
>        else if (ctype != NULL_TREE
> -              && (TREE_CODE (TREE_OPERAND (declarator, 0)) ==
> -                  IDENTIFIER_NODE))
> +              && (identifier_p (TREE_OPERAND (declarator, 0))))
>         {
>           /* Find the list of functions in ctype that have the same
>              name as the declared function.  */
> @@ -6955,7 +6954,7 @@
>
>    gcc_assert (!arglist || TREE_CODE (arglist) == TREE_VEC);
>
> -  if (!is_overloaded_fn (fns) && TREE_CODE (fns) != IDENTIFIER_NODE)
> +  if (!is_overloaded_fn (fns) && !identifier_p (fns))
>      {
>        error ("%q#D is not a function template", fns);
>        return error_mark_node;
> @@ -7058,7 +7057,7 @@
>    spec_entry elt;
>    hashval_t hash;
>
> -  if (TREE_CODE (d1) == IDENTIFIER_NODE)
> +  if (identifier_p (d1))
>      {
>        tree value = innermost_non_namespace_value (d1);
>        if (value && DECL_TEMPLATE_TEMPLATE_PARM_P (value))
> @@ -7853,7 +7852,7 @@
>            || TREE_CODE (t) == TEMPLATE_PARM_INDEX
>            || TREE_CODE (t) == OVERLOAD
>            || BASELINK_P (t)
> -          || TREE_CODE (t) == IDENTIFIER_NODE
> +          || identifier_p (t)
>            || TREE_CODE (t) == TRAIT_EXPR
>            || TREE_CODE (t) == CONSTRUCTOR
>            || CONSTANT_CLASS_P (t))
> @@ -8482,8 +8481,7 @@
>               if (TREE_VALUE (t)
>                   && TREE_CODE (TREE_VALUE (t)) == TREE_LIST
>                   && TREE_VALUE (TREE_VALUE (t))
> -                 && (TREE_CODE (TREE_VALUE (TREE_VALUE (t)))
> -                     == IDENTIFIER_NODE))
> +                 && (identifier_p (TREE_VALUE (TREE_VALUE (t)))))
>                 {
>                   tree chain
>                     = tsubst_expr (TREE_CHAIN (TREE_VALUE (t)), args, complain,
> @@ -13480,7 +13478,7 @@
>                                      input_location);
>         if (error_msg)
>           error (error_msg);
> -       if (!function_p && TREE_CODE (decl) == IDENTIFIER_NODE)
> +       if (!function_p && identifier_p (decl))
>           {
>             if (complain & tf_error)
>               unqualified_name_lookup_error (decl);
> @@ -13907,7 +13905,7 @@
>                                             /*done=*/false,
>                                             /*address_p=*/false);
>           }
> -       else if (koenig_p && TREE_CODE (function) == IDENTIFIER_NODE)
> +       else if (koenig_p && identifier_p (function))
>           {
>             /* Do nothing; calling tsubst_copy_and_build on an identifier
>                would incorrectly perform unqualified lookup again.
> @@ -13991,7 +13989,7 @@
>                     not appropriate, even if an unqualified-name was used
>                     to denote the function.  */
>                  && !DECL_FUNCTION_MEMBER_P (get_first_fn (function)))
> -               || TREE_CODE (function) == IDENTIFIER_NODE)
> +               || identifier_p (function))
>             /* Only do this when substitution turns a dependent call
>                into a non-dependent call.  */
>             && type_dependent_expression_p_push (t)
> @@ -13999,7 +13997,7 @@
>           function = perform_koenig_lookup (function, call_args, false,
>                                             tf_none);
>
> -       if (TREE_CODE (function) == IDENTIFIER_NODE
> +       if (identifier_p (function)
>             && !any_type_dependent_arguments_p (call_args))
>           {
>             if (koenig_p && (complain & tf_warning_or_error))
> @@ -14049,7 +14047,7 @@
>                     function = unq;
>                   }
>               }
> -           if (TREE_CODE (function) == IDENTIFIER_NODE)
> +           if (identifier_p (function))
>               {
>                 if (complain & tf_error)
>                   unqualified_name_lookup_error (function);
> @@ -19781,8 +19779,7 @@
>      return false;
>
>    /* An unresolved name is always dependent.  */
> -  if (TREE_CODE (expression) == IDENTIFIER_NODE
> -      || TREE_CODE (expression) == USING_DECL)
> +  if (identifier_p (expression) || TREE_CODE (expression) == USING_DECL)
>      return true;
>
>    /* Some expression forms are never type-dependent.  */
> @@ -19887,7 +19884,7 @@
>           if (type_dependent_expression_p (TREE_OPERAND (expression, 0)))
>             return true;
>           expression = TREE_OPERAND (expression, 1);
> -         if (TREE_CODE (expression) == IDENTIFIER_NODE)
> +         if (identifier_p (expression))
>             return false;
>         }
>        /* SCOPE_REF with non-null TREE_TYPE is always non-dependent.  */
> @@ -19978,7 +19975,7 @@
>        return NULL_TREE;
>
>      case COMPONENT_REF:
> -      if (TREE_CODE (TREE_OPERAND (*tp, 1)) == IDENTIFIER_NODE)
> +      if (identifier_p (TREE_OPERAND (*tp, 1)))
>         /* In a template, finish_class_member_access_expr creates a
>            COMPONENT_REF with an IDENTIFIER_NODE for op1 even if it isn't
>            type-dependent, so that we can check access control at
> @@ -20222,8 +20219,7 @@
>        || TREE_CODE (tmpl) == TEMPLATE_TEMPLATE_PARM)
>      return true;
>    /* So are names that have not been looked up.  */
> -  if (TREE_CODE (tmpl) == SCOPE_REF
> -      || TREE_CODE (tmpl) == IDENTIFIER_NODE)
> +  if (TREE_CODE (tmpl) == SCOPE_REF || identifier_p (tmpl))
>      return true;
>    /* So are member templates of dependent classes.  */
>    if (TYPE_P (CP_DECL_CONTEXT (tmpl)))
> @@ -20380,7 +20376,7 @@
>       find a TEMPLATE_DECL.  Otherwise, we want to find a TYPE_DECL.  */
>    if (!decl)
>      /*nop*/;
> -  else if (TREE_CODE (TYPENAME_TYPE_FULLNAME (type)) == IDENTIFIER_NODE
> +  else if (identifier_p (TYPENAME_TYPE_FULLNAME (type))
>            && TREE_CODE (decl) == TYPE_DECL)
>      {
>        result = TREE_TYPE (decl);
> Index: search.c
> ===================================================================
> --- search.c    (revision 196891)
> +++ search.c    (working copy)
> @@ -381,7 +381,7 @@
>  {
>    tree field;
>
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    if (TREE_CODE (type) == TEMPLATE_TYPE_PARM
>        || TREE_CODE (type) == BOUND_TEMPLATE_TEMPLATE_PARM
> @@ -1190,7 +1190,7 @@
>        || xbasetype == error_mark_node)
>      return NULL_TREE;
>
> -  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
> +  gcc_assert (identifier_p (name));
>
>    if (TREE_CODE (xbasetype) == TREE_BINFO)
>      {
> Index: semantics.c
> ===================================================================
> --- semantics.c (revision 196891)
> +++ semantics.c (working copy)
> @@ -536,7 +536,7 @@
>  tree
>  finish_goto_stmt (tree destination)
>  {
> -  if (TREE_CODE (destination) == IDENTIFIER_NODE)
> +  if (identifier_p (destination))
>      destination = lookup_label (destination);
>
>    /* We warn about unused labels with -Wunused.  That means we have to
> @@ -1999,7 +1999,7 @@
>      }
>
>    /* Find the name of the overloaded function.  */
> -  if (TREE_CODE (fn) == IDENTIFIER_NODE)
> +  if (identifier_p (fn))
>      identifier = fn;
>    else if (is_overloaded_fn (fn))
>      {
> @@ -2966,7 +2966,7 @@
>           if (scope
>               && (!TYPE_P (scope)
>                   || (!dependent_type_p (scope)
> -                     && !(TREE_CODE (id_expression) == IDENTIFIER_NODE
> +                     && !(identifier_p (id_expression)
>                            && IDENTIFIER_TYPENAME_P (id_expression)
>                            && dependent_type_p (TREE_TYPE (id_expression))))))
>             {
> @@ -2995,8 +2995,7 @@
>          the current class so that we can check later to see if
>          the meaning would have been different after the class
>          was entirely defined.  */
> -      if (!scope && decl != error_mark_node
> -         && TREE_CODE (id_expression) == IDENTIFIER_NODE)
> +      if (!scope && decl != error_mark_node && identifier_p (id_expression))
>         maybe_note_name_used_in_class (id_expression, decl);
>
>        /* Disallow uses of local variables from containing functions, except
> @@ -3169,8 +3168,7 @@
>        /* A template-id where the name of the template was not resolved
>          is definitely dependent.  */
>        else if (TREE_CODE (decl) == TEMPLATE_ID_EXPR
> -              && (TREE_CODE (TREE_OPERAND (decl, 0))
> -                  == IDENTIFIER_NODE))
> +              && (identifier_p (TREE_OPERAND (decl, 0))))
>         dependent_p = true;
>        /* For anything except an overloaded function, just check its
>          type.  */
> @@ -5301,7 +5299,7 @@
>           [expr.ref]), decltype(e) is defined as the type of the entity
>           named by e. If there is no such entity, or e names a set of
>           overloaded functions, the program is ill-formed.  */
> -      if (TREE_CODE (expr) == IDENTIFIER_NODE)
> +      if (identifier_p (expr))
>          expr = lookup_name (expr);
>
>        if (TREE_CODE (expr) == INDIRECT_REF)
> Index: tree.c
> ===================================================================
> --- tree.c      (revision 196891)
> +++ tree.c      (working copy)
> @@ -1734,7 +1734,7 @@
>  tree
>  dependent_name (tree x)
>  {
> -  if (TREE_CODE (x) == IDENTIFIER_NODE)
> +  if (identifier_p (x))
>      return x;
>    if (TREE_CODE (x) != COMPONENT_REF
>        && TREE_CODE (x) != OFFSET_REF
> Index: typeck.c
> ===================================================================
> --- typeck.c    (revision 196891)
> +++ typeck.c    (working copy)
> @@ -2467,7 +2467,7 @@
>              scope, dtor_type);
>        return error_mark_node;
>      }
> -  if (TREE_CODE (dtor_type) == IDENTIFIER_NODE)
> +  if (identifier_p (dtor_type))
>      {
>        /* In a template, names we can't find a match for are still accepted
>          destructor names, and we check them here.  */
> @@ -2588,7 +2588,7 @@
>           dependent_type_p (object_type)
>           /* If NAME is just an IDENTIFIER_NODE, then the expression
>              is dependent.  */
> -         || TREE_CODE (object) == IDENTIFIER_NODE
> +         || identifier_p (object)
>           /* If NAME is "f<args>", where either 'f' or 'args' is
>              dependent, then the expression is dependent.  */
>           || (TREE_CODE (name) == TEMPLATE_ID_EXPR
> @@ -2604,7 +2604,7 @@
>        object = build_non_dependent_expr (object);
>      }
>    else if (c_dialect_objc ()
> -          && TREE_CODE (name) == IDENTIFIER_NODE
> +          && identifier_p (name)
>            && (expr = objc_maybe_build_component_ref (object, name)))
>      return expr;
>
> @@ -2671,8 +2671,7 @@
>             }
>
>           gcc_assert (CLASS_TYPE_P (scope));
> -         gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE
> -                     || TREE_CODE (name) == BIT_NOT_EXPR);
> +         gcc_assert (identifier_p (name) || TREE_CODE (name) == BIT_NOT_EXPR);
>
>           if (constructor_name_p (name, scope))
>             {
> @@ -5067,8 +5066,7 @@
>    arg = mark_lvalue_use (arg);
>    argtype = lvalue_type (arg);
>
> -  gcc_assert (TREE_CODE (arg) != IDENTIFIER_NODE
> -             || !IDENTIFIER_OPNAME_P (arg));
> +  gcc_assert (!identifier_p (arg) || !IDENTIFIER_OPNAME_P (arg));
>
>    if (TREE_CODE (arg) == COMPONENT_REF && type_unknown_p (arg)
>        && !really_overloaded_fn (TREE_OPERAND (arg, 1)))
> Index: typeck2.c
> ===================================================================
> --- typeck2.c   (revision 196891)
> +++ typeck2.c   (working copy)
> @@ -278,8 +278,7 @@
>        void **slot;
>        struct pending_abstract_type *pat;
>
> -      gcc_assert (!decl || DECL_P (decl)
> -                 || TREE_CODE (decl) == IDENTIFIER_NODE);
> +      gcc_assert (!decl || DECL_P (decl) || identifier_p (decl));
>
>        if (!abstract_pending_vars)
>         abstract_pending_vars = htab_create_ggc (31, &pat_calc_hash,
> @@ -336,7 +335,7 @@
>         error ("invalid abstract return type for member function %q+#D", decl);
>        else if (TREE_CODE (decl) == FUNCTION_DECL)
>         error ("invalid abstract return type for function %q+#D", decl);
> -      else if (TREE_CODE (decl) == IDENTIFIER_NODE)
> +      else if (identifier_p (decl))
>         /* Here we do not have location information.  */
>         error ("invalid abstract type %qT for %qE", type, decl);
>        else
> @@ -1241,7 +1240,7 @@
>                  latter case can happen in templates where lookup has to be
>                  deferred.  */
>               gcc_assert (TREE_CODE (ce->index) == FIELD_DECL
> -                         || TREE_CODE (ce->index) == IDENTIFIER_NODE);
> +                         || identifier_p (ce->index));
>               if (ce->index != field
>                   && ce->index != DECL_NAME (field))
>                 {
> @@ -1367,7 +1366,7 @@
>      {
>        if (TREE_CODE (ce->index) == FIELD_DECL)
>         ;
> -      else if (TREE_CODE (ce->index) == IDENTIFIER_NODE)
> +      else if (identifier_p (ce->index))
>         {
>           /* This can happen within a cast, see g++.dg/opt/cse2.C.  */
>           tree name = ce->index;
>

On Fri, Mar 22, 2013 at 10:21:05AM +0100, Richard Biener wrote:
> On Fri, Mar 22, 2013 at 4:50 AM, Gabriel Dos Reis <gdr@axiomatics.org> wrote:
> >
> > This patch introduces identified_p (t) in lieu of
> >
> >    TREE_CODE (t) == IDENTIFIER_NODE
> 
> Generally we have macros like IDENTIFIER_P for this kind of checks.
> 
> > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
> > except that it returns a typed pointer instead of a boolean value.
> 
> Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
> with kind-of dynamic_cast<identifier> (t) (in C++ terms)?  Then
> naming it identifier_p is bad.  We have is-a.h now, so please try to use
> a single style of C++-style casting throughout GCC.

Anyway, if you see better code generated with your change compared to the
old style, I'd say you should file a PR with some preferrably short
preprocessed routine where it makes a difference, because that would show
that either VRP or other optimization pass just isn't doing good job.
It is better code for --enable-checking=yes anyway, right?

	Jakub

Jakub Jelinek <jakub@redhat.com> writes:

| On Fri, Mar 22, 2013 at 10:21:05AM +0100, Richard Biener wrote:
| > On Fri, Mar 22, 2013 at 4:50 AM, Gabriel Dos Reis <gdr@axiomatics.org> wrote:
| > >
| > > This patch introduces identified_p (t) in lieu of
| > >
| > >    TREE_CODE (t) == IDENTIFIER_NODE
| > 
| > Generally we have macros like IDENTIFIER_P for this kind of checks.

I know we have macros for this kind of test, and what what I can tell
looking at the source code, nobody actually bothered using it.  

The point of the change isn't to do that test exactly.  As I explained
earlier, the change was prompted by looking at patterns of usage inf the
existing source code: it is very frequent that we would test XXX_P (t)
and immediately do XXX_GET_YYY (t) when XXX_GET_YYY will do a XXX_CHECK (t),
which morally tests again XXX_P (t), and this is all over the places.
The reason why XXX_GET_YYY (t) does XXX_CHECK (t) is because it could be
mistakenly used on a different node since t isn't "typed".  

| > 
| > > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
| > > except that it returns a typed pointer instead of a boolean value.
| > 
| > Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
| > with kind-of dynamic_cast<identifier> (t) (in C++ terms)?  Then
| > naming it identifier_p is bad.  We have is-a.h now, so please try to use
| > a single style of C++-style casting throughout GCC.
| 
| Anyway, if you see better code generated with your change compared to the
| old style, I'd say you should file a PR with some preferrably short
| preprocessed routine where it makes a difference, because that would show
| that either VRP or other optimization pass just isn't doing good job.

We are talking about files in cp/ right?  They don't know what "short"
means:-) 
With the next change, I'll try to find a short file, although currently
I am working on cp/name-lookup.c

| It is better code for --enable-checking=yes anyway, right?

Yes.

-- Gaby

Richard Biener <richard.guenther@gmail.com> writes:

[...]

| > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
| > except that it returns a typed pointer instead of a boolean value.
| 
| Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
| with kind-of dynamic_cast<identifier> (t) (in C++ terms)?

It would be a mistake to name it dynamic_cast or anything like cast (I
know it is popular in certain C++ circles to name everything xxx_cast),
because dynamic_cast is an implementation detail.  I should probably
have called it "give_me_identifier_pointer_if_operand_points_to_a_cxx_identifier_object"

|  Then
| naming it identifier_p is bad.  We have is-a.h now, so please try to use
| a single style of C++-style casting throughout GCC.

I strongly agree with consistent style,  On the other hand, isn't someone going
to object that is_xxx has a predicate connotation therefore a bad naming
because it isn't returning a bool?

I think a naming that focuses too much on implementation detail is no good,

-- Gaby

On Fri, Mar 22, 2013 at 1:44 PM, Gabriel Dos Reis <gdr@axiomatics.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>
> [...]
>
> | > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
> | > except that it returns a typed pointer instead of a boolean value.
> |
> | Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
> | with kind-of dynamic_cast<identifier> (t) (in C++ terms)?
>
> It would be a mistake to name it dynamic_cast or anything like cast (I
> know it is popular in certain C++ circles to name everything xxx_cast),
> because dynamic_cast is an implementation detail.  I should probably
> have called it "give_me_identifier_pointer_if_operand_points_to_a_cxx_identifier_object"
>
> |  Then
> | naming it identifier_p is bad.  We have is-a.h now, so please try to use
> | a single style of C++-style casting throughout GCC.
>
> I strongly agree with consistent style,  On the other hand, isn't someone going
> to object that is_xxx has a predicate connotation therefore a bad naming
> because it isn't returning a bool?
>
> I think a naming that focuses too much on implementation detail is no good,

Neither is one that is confusing ;)

That said - your specific identifier case should be generalized.  The cgraph
people had exactly the same issue, given a symtab * return a cgraph *
if the symbol is a function, otherwise NULL, combining the previous
if (symbol == function) fn = get-me-a-function (symbol)

And they invented is-a.h as we settled for a template approach which
more readily mimics what the C++ language provides (in form of
static_cast<>, dynamic_cast<>, etc.).

The tree node case is subtly different because we are not actually casting
anything (you are not returning a more specific type than tree) but still
you provide a more C++-ish form to the standard tree.h interfaces.

That is, plain use of is-a.h is possible for your case:

template <>
inline lang_identifier *
as_a (tree p)
{
  if (TREE_CODE (p) == IDENTIFIER_NODE)
    return (lang_identifier *)p;
  return NULL;
}

following existing GCC style (and yes, we've bikeshedded over that
already).

Thus you'd write

   as_a<lang_identifier> (id)

instead of your

   identifier_p (id)

(which should have been lang_identifier_p instead).

Richard.

> -- Gaby

Richard Biener <richard.guenther@gmail.com> writes:

| On Fri, Mar 22, 2013 at 1:44 PM, Gabriel Dos Reis <gdr@axiomatics.org> wrote:
| > Richard Biener <richard.guenther@gmail.com> writes:
| >
| > [...]
| >
| > | > in the C++ front-end.  identifier_p is effectively LANG_IDENTIFIER_CAST
| > | > except that it returns a typed pointer instead of a boolean value.
| > |
| > | Hm?  So you are replacing TREE_CODE (t) == IDENTIFIER_NODE
| > | with kind-of dynamic_cast<identifier> (t) (in C++ terms)?
| >
| > It would be a mistake to name it dynamic_cast or anything like cast (I
| > know it is popular in certain C++ circles to name everything xxx_cast),
| > because dynamic_cast is an implementation detail.  I should probably
| > have called it "give_me_identifier_pointer_if_operand_points_to_a_cxx_identifier_object"
| >
| > |  Then
| > | naming it identifier_p is bad.  We have is-a.h now, so please try to use
| > | a single style of C++-style casting throughout GCC.
| >
| > I strongly agree with consistent style,  On the other hand, isn't someone going
| > to object that is_xxx has a predicate connotation therefore a bad naming
| > because it isn't returning a bool?
| >
| > I think a naming that focuses too much on implementation detail is no good,
| 
| Neither is one that is confusing ;)

You make good points below.

I have one quibble -- since I was provoked into bike shedding :-)

| 
| That said - your specific identifier case should be generalized.  The cgraph
| people had exactly the same issue, given a symtab * return a cgraph *
| if the symbol is a function, otherwise NULL, combining the previous
| if (symbol == function) fn = get-me-a-function (symbol)
| 
| And they invented is-a.h as we settled for a template approach which
| more readily mimics what the C++ language provides (in form of
| static_cast<>, dynamic_cast<>, etc.).
| 
| The tree node case is subtly different because we are not actually casting
| anything (you are not returning a more specific type than tree) but still
| you provide a more C++-ish form to the standard tree.h interfaces.
| 
| That is, plain use of is-a.h is possible for your case:
| 
| template <>
| inline lang_identifier *
| as_a (tree p)
| {
|   if (TREE_CODE (p) == IDENTIFIER_NODE)
|     return (lang_identifier *)p;
|   return NULL;
| }
| 
| following existing GCC style (and yes, we've bikeshedded over that
| already).

I would have dropped the suffix "_a" on both "is_a" and "as_a", did
I get a chance to bike shed when is-a.h was introduced.

I will add the specialization and use it the appropriate places.

| 
| Thus you'd write
| 
|    as_a<lang_identifier> (id)
| 
| instead of your
| 
|    identifier_p (id)
| 
| (which should have been lang_identifier_p instead).

-- Gaby