Comments

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 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