| Message ID | 5243025E.8050101@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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
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.
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.
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
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
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
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.
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
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
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
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.
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. */
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.