Patchwork Trivial cleanup

login
register
mail settings
Submitter Jeff Law
Date Sept. 25, 2013, 3:33 p.m.
Message ID <5243025E.8050101@redhat.com>
Download mbox | patch
Permalink /patch/277901/
State New
Headers show

Comments

Jeff Law - Sept. 25, 2013, 3:33 p.m.
I was looking at the vec class to figure out the best way to do some 
things and realized we have a "last" member function.  Using foo.last() 
is clearer than foo[foo.length() - 1]

On a related note, our current standards require a space between a 
function name and the open paren for its argument list.  However, it 
leads to fugly code in some cases.  Assume foo is an vec instance and we 
want to look at something in the last vector element.  Our current 
standards would imply something like this:

foo.last ()->bar


Which is how the current patch formats that code.  However, I typically 
see this more often in C++ codebases as

foo.last()->bar

But then, what if we just want the last element without peeking deeper 
inside it?

foo.last ()?

or

foo.last()?


Do we want to make any changes in our style guidelines to handle this 
better?

Anyway, bootstrapped & regression tested on x86_64-unknown-linux-gnu. 
Installed onto the trunk.

Jeff
* tree-ssa-threadedge.c (thread_across_edge): Use foo.last () rather
        than foo[foo.length () - 1] to access last member in a vec.
        * tree-ssa-threadupdate.c (register_jump_thread): Similarly.
Andrew MacLeod - Sept. 25, 2013, 3:46 p.m.
On 09/25/2013 11:33 AM, Jeff Law wrote:
>
> I was looking at the vec class to figure out the best way to do some 
> things and realized we have a "last" member function.  Using 
> foo.last() is clearer than foo[foo.length() - 1]
>
> On a related note, our current standards require a space between a 
> function name and the open paren for its argument list.  However, it 
> leads to fugly code in some cases.  Assume foo is an vec instance and 
> we want to look at something in the last vector element.  Our current 
> standards would imply something like this:
>
> foo.last ()->bar
>
>
> Which is how the current patch formats that code.  However, I 
> typically see this more often in C++ codebases as
>
> foo.last()->bar
>
> But then, what if we just want the last element without peeking deeper 
> inside it?
>
> foo.last ()?
>
> or
>
> foo.last()?
>
>
> Do we want to make any changes in our style guidelines to handle this 
> better?

I noticed that with the wrapper conversion, often you will get a 
sequence of 3 or more method calls, and its quite unbearable to have the 
spaces.
simple things like
   int unsignedsrcp = ptrvar.type().type().type_unsigned();
vs
   int unsignedsrcp = ptrvar.type ().type ().type_unsigned ();

I was going to bring it up at some point too.  My preference is strongly 
to simply eliminate the space on methods...

Andrew
Paolo Carlini - Sept. 25, 2013, 3:48 p.m.
On 9/25/13 10:46 AM, Andrew MacLeod wrote:
> I was going to bring it up at some point too.  My preference is 
> strongly to simply eliminate the space on methods...
Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.

Paolo.
Jeff Law - Sept. 25, 2013, 3:59 p.m.
On 09/25/2013 09:48 AM, Paolo Carlini wrote:
> On 9/25/13 10:46 AM, Andrew MacLeod wrote:
>> I was going to bring it up at some point too.  My preference is
>> strongly to simply eliminate the space on methods...
> Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.
Yea.  I actually reviewed the libstdc++ guidelines to see where they 
differed from GNU's C guidelines.

I'm strongly in favor of dropping the horizontal whitespace between the 
method name and its open paren when the result is then dereferenced.  ie
foo.last()->e rather than foo.last ()->e.

However, do we want an exception when the result isn't immediately 
dereferenced?  ie, foo.last () or foo.last()?

jeff
Jakub Jelinek - Sept. 25, 2013, 4:04 p.m.
On Wed, Sep 25, 2013 at 11:46:12AM -0400, Andrew MacLeod wrote:
> I noticed that with the wrapper conversion, often you will get a
> sequence of 3 or more method calls, and its quite unbearable to have
> the spaces.
> simple things like
>   int unsignedsrcp = ptrvar.type().type().type_unsigned();
> vs
>   int unsignedsrcp = ptrvar.type ().type ().type_unsigned ();
> 
> I was going to bring it up at some point too.  My preference is
> strongly to simply eliminate the space on methods...

My preference is to keep the spaces, it makes code more readable,
no space before ( looks very ugly to me.

	Jakub
Jeff Law - Sept. 25, 2013, 7:18 p.m.
On 09/25/2013 10:04 AM, Jakub Jelinek wrote:
> On Wed, Sep 25, 2013 at 11:46:12AM -0400, Andrew MacLeod wrote:
>> I noticed that with the wrapper conversion, often you will get a
>> sequence of 3 or more method calls, and its quite unbearable to have
>> the spaces.
>> simple things like
>>    int unsignedsrcp = ptrvar.type().type().type_unsigned();
>> vs
>>    int unsignedsrcp = ptrvar.type ().type ().type_unsigned ();
>>
>> I was going to bring it up at some point too.  My preference is
>> strongly to simply eliminate the space on methods...
>
> My preference is to keep the spaces, it makes code more readable,
> no space before ( looks very ugly to me.
I generally find the lack of a space before the open-paren ugly as well, 
but I find something like Andrew's example with the horizontal spaces 
insanely bad.
Jeff
Jakub Jelinek - Sept. 25, 2013, 7:31 p.m.
On Wed, Sep 25, 2013 at 01:18:06PM -0600, Jeff Law wrote:
> On 09/25/2013 10:04 AM, Jakub Jelinek wrote:
> >On Wed, Sep 25, 2013 at 11:46:12AM -0400, Andrew MacLeod wrote:
> >>I noticed that with the wrapper conversion, often you will get a
> >>sequence of 3 or more method calls, and its quite unbearable to have
> >>the spaces.
> >>simple things like
> >>   int unsignedsrcp = ptrvar.type().type().type_unsigned();
> >>vs
> >>   int unsignedsrcp = ptrvar.type ().type ().type_unsigned ();
> >>
> >>I was going to bring it up at some point too.  My preference is
> >>strongly to simply eliminate the space on methods...
> >
> >My preference is to keep the spaces, it makes code more readable,
> >no space before ( looks very ugly to me.
> I generally find the lack of a space before the open-paren ugly as
> well, but I find something like Andrew's example with the horizontal
> spaces insanely bad.

Then perhaps we don't want the above style and write
int unsignedsrcp = type_unsigned (type (type (ptrvar)));
instead?  If we really need to have C++ uglification everywhere, perhaps
through:
template <typename T>
tree
type (T t)
{
  return t.get_type (t);
}
or similar.

	Jakub
Michael Matz - Sept. 26, 2013, 2:15 p.m.
Hi,

On Wed, 25 Sep 2013, Jeff Law wrote:

> > > I was going to bring it up at some point too.  My preference is
> > > strongly to simply eliminate the space on methods...
> > Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.
> Yea.  I actually reviewed the libstdc++ guidelines to see where they differed
> from GNU's C guidelines.
> 
> I'm strongly in favor of dropping the horizontal whitespace between the 
> method name and its open paren when the result is then dereferenced.  
> ie foo.last()->e rather than foo.last ()->e.

I'd prefer to not write in this style at all, like Jakub.  If we must 
absolutely have it, then I agree that the space before _empty_ parentheses 
are ugly if followed by references.  I.e. I'd like to see spaces before 
parens as is customary, except in one case: empty parens in the middle of 
expressions (which don't happen very often right now in GCC, and hence 
wouldn't introduce a coding style confusion):

do.this ();
give.that()->flag;
get.list (one)->clear ();

I'd prefer to not have further references to return values be applied, 
though (as in, the parentheses should be the end of statement), which 
would avoid the topic (at the expensive of having to invent names for 
those temporaries, or to write trivial wrapper methods contracting several 
method calls).


Ciao,
Michael.
Richard Guenther - Sept. 26, 2013, 2:21 p.m.
On Thu, Sep 26, 2013 at 4:15 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 25 Sep 2013, Jeff Law wrote:
>
>> > > I was going to bring it up at some point too.  My preference is
>> > > strongly to simply eliminate the space on methods...
>> > Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.
>> Yea.  I actually reviewed the libstdc++ guidelines to see where they differed
>> from GNU's C guidelines.
>>
>> I'm strongly in favor of dropping the horizontal whitespace between the
>> method name and its open paren when the result is then dereferenced.
>> ie foo.last()->e rather than foo.last ()->e.
>
> I'd prefer to not write in this style at all, like Jakub.  If we must
> absolutely have it, then I agree that the space before _empty_ parentheses
> are ugly if followed by references.  I.e. I'd like to see spaces before
> parens as is customary, except in one case: empty parens in the middle of
> expressions (which don't happen very often right now in GCC, and hence
> wouldn't introduce a coding style confusion):
>
> do.this ();
> give.that()->flag;
> get.list (one)->clear ();
>
> I'd prefer to not have further references to return values be applied,
> though (as in, the parentheses should be the end of statement), which
> would avoid the topic (at the expensive of having to invent names for
> those temporaries, or to write trivial wrapper methods contracting several
> method calls).

Seconded, even give.that()->flag; is ugly.

Richard.
Andrew MacLeod - Sept. 26, 2013, 9:52 p.m.
On 09/26/2013 10:21 AM, Richard Biener wrote:
> On Thu, Sep 26, 2013 at 4:15 PM, Michael Matz <matz@suse.de> wrote:
>> Hi,
>>
>> On Wed, 25 Sep 2013, Jeff Law wrote:
>>
>>>>> I was going to bring it up at some point too.  My preference is
>>>>> strongly to simply eliminate the space on methods...
>>>> Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.
>>> Yea.  I actually reviewed the libstdc++ guidelines to see where they differed
>>> from GNU's C guidelines.
>>>
>>> I'm strongly in favor of dropping the horizontal whitespace between the
>>> method name and its open paren when the result is then dereferenced.
>>> ie foo.last()->e rather than foo.last ()->e.
>> I'd prefer to not write in this style at all, like Jakub.  If we must
>> absolutely have it, then I agree that the space before _empty_ parentheses
>> are ugly if followed by references.  I.e. I'd like to see spaces before
>> parens as is customary, except in one case: empty parens in the middle of
>> expressions (which don't happen very often right now in GCC, and hence
>> wouldn't introduce a coding style confusion):
>>
>> do.this ();
>> give.that()->flag;
>> get.list (one)->clear ();
>>
>> I'd prefer to not have further references to return values be applied,
>> though (as in, the parentheses should be the end of statement), which
>> would avoid the topic (at the expensive of having to invent names for
>> those temporaries, or to write trivial wrapper methods contracting several
>> method calls).
> Seconded, even give.that()->flag; is ugly.
>
I don't find it ugly :-)

so my example would end up being
    int unsignedsrcp = ptrvar.type().type().type_unsigned ();  ?
I can live with this suggestion...  its the sequence of 2 or 3 or 4 
empty parens with spaces that I really found distasteful.

Andrew
Jeff Law - Sept. 27, 2013, 5:03 a.m.
On 09/26/2013 08:15 AM, Michael Matz wrote:
> Hi,
>
> On Wed, 25 Sep 2013, Jeff Law wrote:
>
>>>> I was going to bring it up at some point too.  My preference is
>>>> strongly to simply eliminate the space on methods...
>>> Which wouldn't be so weird: in the libstdc++-v3 code we do it all the time.
>> Yea.  I actually reviewed the libstdc++ guidelines to see where they differed
>> from GNU's C guidelines.
>>
>> I'm strongly in favor of dropping the horizontal whitespace between the
>> method name and its open paren when the result is then dereferenced.
>> ie foo.last()->e rather than foo.last ()->e.
>
> I'd prefer to not write in this style at all, like Jakub.  If we must
> absolutely have it, then I agree that the space before _empty_ parentheses
> are ugly if followed by references.  I.e. I'd like to see spaces before
> parens as is customary, except in one case: empty parens in the middle of
> expressions (which don't happen very often right now in GCC, and hence
> wouldn't introduce a coding style confusion):
>
> do.this ();
> give.that()->flag;
> get.list (one)->clear ();
>
> I'd prefer to not have further references to return values be applied,
> though (as in, the parentheses should be the end of statement), which
> would avoid the topic (at the expensive of having to invent names for
> those temporaries, or to write trivial wrapper methods contracting several
> method calls).
Should we consider banning dereferencing the result of a method call and 
instead prefer to use a more functional interface such as Jakub has 
suggested, or have the result of the method call put into a temporary 
and dereference the temporary.

I considered suggesting the latter.  I wouldn't be a huge fan of the 
unnecessary temporaries, but they may be better than the horrid 
foo.last()->argh()->e->src or whatever.

Stuffing the result into a temporary does have one advantage, it 
encourages us to CSE across the method calls in cases where the compiler 
might not be able to do so.  Of course, being humans, we'll probably 
mess it up.

jeff
Andrew MacLeod - Sept. 28, 2013, 2:31 p.m.
On 09/27/2013 01:03 AM, Jeff Law wrote:
> On 09/26/2013 08:15 AM, Michael Matz wrote:
>> Hi,
>>
>> On Wed, 25 Sep 2013, Jeff Law wrote:
>>
>>>>> I was going to bring it up at some point too.  My preference is
>>>>> strongly to simply eliminate the space on methods...
>>>> Which wouldn't be so weird: in the libstdc++-v3 code we do it all 
>>>> the time.
>>> Yea.  I actually reviewed the libstdc++ guidelines to see where they 
>>> differed
>>> from GNU's C guidelines.
>>>
>>> I'm strongly in favor of dropping the horizontal whitespace between the
>>> method name and its open paren when the result is then dereferenced.
>>> ie foo.last()->e rather than foo.last ()->e.
>>
>> I'd prefer to not write in this style at all, like Jakub.  If we must
>> absolutely have it, then I agree that the space before _empty_ 
>> parentheses
>> are ugly if followed by references.  I.e. I'd like to see spaces before
>> parens as is customary, except in one case: empty parens in the 
>> middle of
>> expressions (which don't happen very often right now in GCC, and hence
>> wouldn't introduce a coding style confusion):
>>
>> do.this ();
>> give.that()->flag;
>> get.list (one)->clear ();
>>
>> I'd prefer to not have further references to return values be applied,
>> though (as in, the parentheses should be the end of statement), which
>> would avoid the topic (at the expensive of having to invent names for
>> those temporaries, or to write trivial wrapper methods contracting 
>> several
>> method calls).
> Should we consider banning dereferencing the result of a method call 
> and instead prefer to use a more functional interface such as Jakub 
> has suggested, or have the result of the method call put into a 
> temporary and dereference the temporary.
>
> I considered suggesting the latter.  I wouldn't be a huge fan of the 
> unnecessary temporaries, but they may be better than the horrid 
> foo.last()->argh()->e->src or whatever.
>
> Stuffing the result into a temporary does have one advantage, it 
> encourages us to CSE across the method calls in cases where the 
> compiler might not be able to do so.  Of course, being humans, we'll 
> probably mess it up.
>
> jeff
I don't like the more functional interface... I thought the suggestion 
might be a little tongue in cheek, but wasn't sure :-)  I can't imagine 
the number of templates that would introduce... and the impact on 
compile/link time would probably not be trivial.

temps would be OK with me, but there are a couple of concerns.
  - I'd want to be able to declare the temps at the point of use, not 
the top of the function. this would actually help with clarity I think. 
Not sure what the current coding standard says about that.
  - the compiler better do an awesome job of sharing stack  space for 
user variables in a function... I wouldn't want to blow up the stack 
with a bazillion unrelatd temps each wit their own location.

My example in this form would look something like:
int unsignedsrcp = ptrvar.type().type().type_unsigned();

<...>
GimpleType t1 = ptrvar.type ();
GimpleType t2 = t1.type ();
int unsignedsrcp = t2.type.unsigned ();

And yes, we'll probably introduce the odd human CSE error.. hopefully 
the test suite will catch them :-)

I think I still prefer matz's suggestion, but I could be on board with 
this one too.   some expressions are crazy complicated

Andrew
Michael Matz - Sept. 30, 2013, 8:05 a.m.
Hi,

On Sat, 28 Sep 2013, Andrew MacLeod wrote:

> My example in this form would look something like:
> int unsignedsrcp = ptrvar.type().type().type_unsigned();
> 
> <...>
> GimpleType t1 = ptrvar.type ();
> GimpleType t2 = t1.type ();

Stop that CamelCase dyslexia already, will you?  ;-)


Ciao,
Michael.
Andrew MacLeod - Sept. 30, 2013, 2:12 p.m.
On 09/30/2013 04:05 AM, Michael Matz wrote:
> Hi,
>
> On Sat, 28 Sep 2013, Andrew MacLeod wrote:
>
>> My example in this form would look something like:
>> int unsignedsrcp = ptrvar.type().type().type_unsigned();
>>
>> <...>
>> GimpleType t1 = ptrvar.type ();
>> GimpleType t2 = t1.type ();
> Stop that CamelCase dyslexia already, will you?  ;-)
>
>
:-)  Im using it purely as a holding place during the prototyping :-)

  Im guessing as a project we dont want it (Ive grown accustomed to it, 
but I'm ambivalent to it) ,   For now it does allow me to search/replace 
project wide without getting false hits since there is no other 
CamelCase.  When Im done the header file refactoring I was going to 
discuss what we want for names and conventions before really getting 
going :-)

Andrew
Jeff Law - Sept. 30, 2013, 4:38 p.m.
On 09/30/13 02:05, Michael Matz wrote:
> Hi,
>
> On Sat, 28 Sep 2013, Andrew MacLeod wrote:
>
>> My example in this form would look something like:
>> int unsignedsrcp = ptrvar.type().type().type_unsigned();
>>
>> <...>
>> GimpleType t1 = ptrvar.type ();
>> GimpleType t2 = t1.type ();
>
> Stop that CamelCase dyslexia already, will you?  ;-)
:-)

I don't think anyone is suggesting CamelCase for GCC; Andrew has been 
using it a lot lately in the reorganizational work so that he can 
quickly find things that will need changing again.  A grep for 
"GimpleType" is a lot more useful than a grep on gimple or type ;-)

jeff
Jeff Law - Sept. 30, 2013, 4:48 p.m.
On 09/28/13 08:31, Andrew MacLeod wrote:
>
> temps would be OK with me, but there are a couple of concerns.
>   - I'd want to be able to declare the temps at the point of use, not
> the top of the function. this would actually help with clarity I think.
> Not sure what the current coding standard says about that.
Point of use is fine for GCC now.   From our coding conventions:

Variable Definitions

Variables should be defined at the point of first use, rather than at 
the top of the function. The existing code obviously does not follow 
that rule, so variables may be defined at the top of the function, as in 
C90.

Variables may be simultaneously defined and tested in control expressions.





>   - the compiler better do an awesome job of sharing stack  space for
> user variables in a function... I wouldn't want to blow up the stack
> with a bazillion unrelatd temps each wit their own location.
If the objects have the same type and disjoint lifetimes, they can be 
easily shared.

Things are more difficult if the types are different -- IIRC, the root 
of the problem is the optimizers can interchange a load of one type with 
a later store of the other -- the aliasing code says "hey, they're 
different types, so they don't alias, feel free to move them around as 
desired" and all hell breaks loose.

Jeff
Michael Matz - Oct. 1, 2013, 1:12 p.m.
Hi,

On Mon, 30 Sep 2013, Jeff Law wrote:

> >   - the compiler better do an awesome job of sharing stack  space for
> > user variables in a function... I wouldn't want to blow up the stack
> > with a bazillion unrelatd temps each wit their own location.
> If the objects have the same type and disjoint lifetimes, they can be easily
> shared.
> 
> Things are more difficult if the types are different

Not anymore.  We adjust the alias machinery when merging differently typed 
variables into the same stack slot, see update_alias_info_with_stack_vars.

> -- IIRC, the root 
> of the problem is the optimizers can interchange a load of one type with 
> a later store of the other -- the aliasing code says "hey, they're 
> different types, so they don't alias, feel free to move them around as 
> desired" and all hell breaks loose.

That was the problem once, yes.  Meanwhile we should have fairly decent 
stack slot reuse, especially with variables declared in different scopes 
(since the end-of-scope CLOBBERs), for non-SSA_NAME temps that is.  For 
the others it's the register allocator anyway that has to do a decent job 
(and it does).


Ciao,
Michael.

Patch

diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 47db280..2ca56342 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -947,8 +947,7 @@  thread_across_edge (gimple dummy_cond,
 	    }
 
 	  remove_temporary_equivalences (stack);
-	  propagate_threaded_block_debug_into (path[path.length () - 1]->dest,
-					       e->dest);
+	  propagate_threaded_block_debug_into (path.last ()->dest, e->dest);
 	  register_jump_thread (path, false);
 	  path.release ();
 	  return;
@@ -987,7 +986,7 @@  thread_across_edge (gimple dummy_cond,
 	path.safe_push (taken_edge);
 	found = false;
 	if ((e->flags & EDGE_DFS_BACK) == 0
-	    || ! cond_arg_set_in_bb (path[path.length () - 1], e->dest))
+	    || ! cond_arg_set_in_bb (path.last (), e->dest))
 	  found = thread_around_empty_blocks (taken_edge,
 					      dummy_cond,
 					      handle_dominating_asserts,
@@ -999,7 +998,7 @@  thread_across_edge (gimple dummy_cond,
 	   record the jump threading opportunity.  */
 	if (found)
 	  {
-	    propagate_threaded_block_debug_into (path[path.length () - 1]->dest,
+	    propagate_threaded_block_debug_into (path.last ()->dest,
 						 taken_edge->dest);
 	    register_jump_thread (path, true);
 	  }
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 4131128..fd5234c 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -1401,7 +1401,7 @@  register_jump_thread (vec<edge> path, bool through_joiner)
   if (!through_joiner)
     e3 = NULL;
   else
-    e3 = path[path.length () - 1];
+    e3 = path.last ();
 
   /* This can occur if we're jumping to a constant address or
      or something similar.  Just get out now.  */