On 11/19/2014 05:28 PM, David Malcolm wrote:
On Wed, 2014-11-19 at 17:24 -0500, David Malcolm wrote:
On Wed, 2014-11-19 at 22:36 +0100, Richard Biener wrote:
On November 19, 2014 10:09:56 PM CET, Andrew MacLeod <amacl...@redhat.com>
wrote:
On 11/19/2014 03:43 PM, Richard Biener wrote:
On November 19, 2014 8:26:23 PM CET, Andrew MacLeod
<amacl...@redhat.com> wrote:
On 11/19/2014 01:12 PM, David Malcolm wrote:
(A) could become:
greturn *stmt = gsi->as_a_greturn ();
(B) could become:
stmt = gsi->dyn_cast <gcall *> ();
if (!stmt)
or:
stmt = gsi->dyn_cast_gcall ();
if (!stmt)
or maybe:
stmt = gsi->is_a_gcall ();
if (!stmt)
An earlier version of my patches had casting methods within the
gimple_statement_base class, which were rejected; the above
proposals
would instead put them within gimple_stmt_iterator.
I would like all gsi routines to be consistent, not a mix of
functions
and methods.
so something like
stmt = gsi_as_call (gsi);
stmt = gsi_dyn_call (gsi);
or we need to change gsi_stmt() and friends into methods and access
them
as gsi->stmt ().. which is possibly better, but that much more
intrusive again (another 2000+ locations).
If we switched to methods everywhere for gsi, I'd prefer something
like
gsi->as_a_call ()
gsi->is_a_call ()
gsi->dyn_cast_call ()
I think its more readable... and it removes a dependency on the
implementation.. so if we ever decide to change the name of 'gcall'
down
the road to using a namespace, and make it gimple::call or whatever,
we
wont have to change every single gsi-> location which has a
templated
use of the type.
I'm also think this sort of thing could probably wait until next
stage
1..
my 2 cents...
Why not as_a <gassign *> (*gsi)? It would
Add operator* to gsi of course.
Richard.
I could live with that form too.
we often have an instance of gimple_stmt_iterator rather than a pointer
to it, so wouldn't "operator gimple *()" to implicitly call gsi_stmt()
when needed work better? (or "operator gimple ()" before the next
change) ..
Not sure. The * matches how iterators work in STL...
Note that for the cases where we pass a pointer to an iterator we can change
those to use references to avoid writing **gsi.
Richard.
Andrew
I had a go at adding an operator * to gimple_stmt_iterator, using it
everywhere that we do an as_a<> or dyn_cast<> on the result of a
gsi_stmt, to abbreviate the gsi_stmt call down to one character.
Patch attached; only lightly smoketested; am posting it for the sake of
discussion.
I don't think this API will make the non-C++-fans happier; I think the
objection to the work I just merged is that it's adding more C++ than
those people are comfortable with.
So although the attached patch makes things shorter (good), it's taking
things in a "more C++" direction (questionable). I'd hoped to paper
over the C++ somewhat.
I suspect that any API which requires the of < > characters within the
implementation of a gimple pass to mean a template is going to give
those less happy with C++ a visceral "ugh" reaction. I wonder if
there's a way to spell these things that's concise and which doesn't
involve <> ?
Answering my own question, user-defined conversion operators, though
should they as_a or dyn_cast?
Here's an example where they mean "as_a", radically shortening the
conversion (shorter, in fact, that the pre-merger code):
diff --git a/gcc/asan.c b/gcc/asan.c
index be28ede..890e58b 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1902,7 +1902,7 @@ instrument_builtin_call (gimple_stmt_iterator *iter)
return false;
bool iter_advanced_p = false;
- gcall *call = as_a <gcall *> (gsi_stmt (*iter));
+ gcall *call = *iter;
gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL));
diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index fb6cc07..52106fa 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -33,6 +33,8 @@ struct gimple_stmt_iterator
block/sequence is removed. */
gimple_seq *seq;
basic_block bb;
+
+ operator gcall * ();
};
/* Iterator over GIMPLE_PHI statements. */
@@ -331,4 +333,11 @@ gsi_seq (gimple_stmt_iterator i)
return *i.seq;
}
+inline
+gimple_stmt_iterator::operator gcall * ()
+{
+ return as_a <gcall *> (gsi_stmt (*this));
+}
+
+
#endif /* GCC_GIMPLE_ITERATOR_H */
(again, I only checked that it compiles)
Dave
I think the problem is different people will naturally think they should
do different things, and I see the logic to both.
In my various tree experiments, for a while I used the user defined
conversion as a dyn_cast because that is what seemed "right" to me. I
can assure you that can be flawed... mostly because if its something you
didn't anticipate, you just get a NULL back and things proceed... in
particular when calling functions. When I disabled that
auto-conversion, I discovered a whole bunch of code that was silently wrong.
At least with as_a you will catch it immediately during compilation if
things are wrong... but it still feels a little wrong to me. Likewise
you could overload the operator= to do similar things but you wouldn't
get unexpected auto-conversions as function parameters.
SInce there are multiple ways of viewing it, I think its probably better
to be explicit somehow.
Andrew
PS. this whole area sounds like a "best practices" decent C++ projects
probably figured out a decade ago... or at least should have :-)
