| Message ID | alpine.LNX.2.00.1309181640400.9949@wotan.suse.de |
|---|---|
| State | New |
| Headers | show |
On 09/18/2013 10:24 AM, Michael Matz wrote: > > I'm irritated by the member name uglification (e.g. equiv_stack_ with > trailing underscore). I know that's a certain style to mark private > members, but I think it's a bad style (like prefixing variable names with > their type), and before it sets a precedent in GCCs c++ coding style I'd > like this to be changed, like in the below. We're already using the trailing underscore idiom for private objects moving into classes (see the pass class). Furthermore, we've used similar idioms in the past when moving objects from the global namespace into structures (the x_ prefixing you'll find in various structures). You really should have chimed in before now. > > I'd also like us to not use member privatization in our classes, but > that's not in the patch, but if we could agree on that it would be nice. Member privatization is quite natural. What specifically do you not like about the practice? > > Regstrapped on x86-64-linux, okay? Obviously any ChangeLog, formatting and such can go in. However, the trailing underscore should stay given it's already established practice and has several nice benefits. jeff
Hi, On Wed, 18 Sep 2013, Jeff Law wrote: > On 09/18/2013 10:24 AM, Michael Matz wrote: > > > > I'm irritated by the member name uglification (e.g. equiv_stack_ with > > trailing underscore). I know that's a certain style to mark private > > members, but I think it's a bad style (like prefixing variable names with > > their type), and before it sets a precedent in GCCs c++ coding style I'd > > like this to be changed, like in the below. > > We're already using the trailing underscore idiom for private objects > moving into classes (see the pass class). I know, and I don't like it there either. > Furthermore, we've used similar idioms in the past when moving objects > from the global namespace into structures (the x_ prefixing you'll find > in various structures). Yes, but that is usually done to not confuse ourself when there are accessor macros of the same name as the accessed member/variable, i.e. when we want to make really sure that we never, from no code, access the variable itself. I.e. the uglification serves the purpose of avoiding access to that name. That's not the case with these class members. They are quite clearly accessed in all the member functions, and they are intended to be accessed from them. So they should have no uglified name. > You really should have chimed in before now. Yes I should have, but better late than never. As of now it's no trend of uglification yet, the only preexisting cases of the same type (uglified class member names) are the pass manager and bit_field_mode_iterator, both fairly recent. The other case (vec.h) should have been changed to non-ugly names when it was converted to classes. In any case we should not introduce more of these. > > I'd also like us to not use member privatization in our classes, but > > that's not in the patch, but if we could agree on that it would be nice. > Member privatization is quite natural. What specifically do you not like > about the practice? http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00302.html That was conditional on "when we need to jump through hoops", but for constistency it'd make sense to avoid it everywhere. (I know that Ian agreed to that mail, but somehow the mailing list archives don't have that!?) > > Regstrapped on x86-64-linux, okay? > > Obviously any ChangeLog, formatting and such can go in. However, the > trailing underscore should stay given it's already established practice > and has several nice benefits. What's the benefit of reading and writing such noisy lines? : *out_mode = mode_; mode_ = GET_MODE_WIDER_MODE (mode_); count_++; The uglification merely makes code harder to write and read, it should be used in cases where you _don't_ want developers to write such names. Ciao, Michael.
On Wed, Sep 18, 2013 at 07:17:35PM +0200, Michael Matz wrote: > > > I'd also like us to not use member privatization in our classes, but > > > that's not in the patch, but if we could agree on that it would be nice. > > Member privatization is quite natural. What specifically do you not like > > about the practice? > > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00302.html > > That was conditional on "when we need to jump through hoops", but for > constistency it'd make sense to avoid it everywhere. If you need to jump through a bunch of hoops then the language feature isn't useful in that case, but not using it in other cases where it actually is useful only to be consistant seems pretty foolish. Especially if you ban things like fake deletion of members by making them private, or make things public and add an x_ prefix that does the same thing as private but in an uglier way with more manual work. > > > Regstrapped on x86-64-linux, okay? > > > > Obviously any ChangeLog, formatting and such can go in. However, the > > trailing underscore should stay given it's already established practice > > and has several nice benefits. > > What's the benefit of reading and writing such noisy lines? : > > *out_mode = mode_; > mode_ = GET_MODE_WIDER_MODE (mode_); > count_++; > > The uglification merely makes code harder to write and read, it should be > used in cases where you _don't_ want developers to write such names. I don't care for the _ thing too much, but I think it is useful for seeing where a variable comes from, and I don't think you can seriously argue it makes reading or writing much harder. Trev > > > Ciao, > Michael.
On Wed, Sep 18, 2013 at 10:17 AM, Michael Matz <matz@suse.de> wrote: > >> > I'd also like us to not use member privatization in our classes, but >> > that's not in the patch, but if we could agree on that it would be nice. > >> Member privatization is quite natural. What specifically do you not like >> about the practice? > > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00302.html > > That was conditional on "when we need to jump through hoops", but for > constistency it'd make sense to avoid it everywhere. > (I know that Ian agreed to that mail, but somehow the mailing list > archives don't have that!?) I accidentally sent the e-mail in HTML mode, and it bounced. I didn't think it was important enough to resend. But I wouldn't go so far as to say that we should never use private members, just that I think it's better to use public members than to contort the code to force them to be private. In any case I do think that any discussion of this area should be with regard to suggested changes to the coding conventions at http://gcc.gnu.org/codingconventions.html . Those conventions do say "Prefer to make data members private" and "When structs and/or classes have member functions, prefer to name data members with a trailing underscore." Ian
On 09/18/2013 11:17 AM, Michael Matz wrote: > Hi, > > On Wed, 18 Sep 2013, Jeff Law wrote: > >> On 09/18/2013 10:24 AM, Michael Matz wrote: >>> >>> I'm irritated by the member name uglification (e.g. equiv_stack_ with >>> trailing underscore). I know that's a certain style to mark private >>> members, but I think it's a bad style (like prefixing variable names with >>> their type), and before it sets a precedent in GCCs c++ coding style I'd >>> like this to be changed, like in the below. >> >> We're already using the trailing underscore idiom for private objects >> moving into classes (see the pass class). > > I know, and I don't like it there either. Well, as Ian pointed out, it is in our recommended style guidelines and you'll find uses in the vec class as well. It's well established at this point and I see no compelling reason to go back unless you can convince the project as a whole to change the C++ guidelines. > >>> I'd also like us to not use member privatization in our classes, but >>> that's not in the patch, but if we could agree on that it would be nice. >> Member privatization is quite natural. What specifically do you not like >> about the practice? > > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00302.html > > That was conditional on "when we need to jump through hoops", but for > constistency it'd make sense to avoid it everywhere. > (I know that Ian agreed to that mail, but somehow the mailing list > archives don't have that!?) I don't see anything in Trevor's work that requires jumping through hoops. It's pretty simple stuff. And again, as Ian pointed out, our established guidelines for C++ usage encourage this behaviour. > >>> Regstrapped on x86-64-linux, okay? >> >> Obviously any ChangeLog, formatting and such can go in. However, the >> trailing underscore should stay given it's already established practice >> and has several nice benefits. > > What's the benefit of reading and writing such noisy lines? : > > *out_mode = mode_; > mode_ = GET_MODE_WIDER_MODE (mode_); > count_++; It makes it very clear to the reader that we're dealing with objects that belong to a class instance rather than direct access to an auto or static. That can be important. > > The uglification merely makes code harder to write and read, it should be > used in cases where you _don't_ want developers to write such names. I feel it makes it harder in some ways and easier in others. Given it's recommended by our C++ guidelines which were discussed at length, I'm going to explicitly NAK your patch. If you want to re-open the guidelines for C++ usage, then that's fine with me and if we as a project change the guidelines to disallow such things, then that will be the time to remove the trailing underscores, private members, etc. FWIW, I have worked on large C++ codebases that were a free-for-all and found them *amazingly* painful. The restricted set allowed for GCC is actually quite reasonable IMHO, particularly for projects where the main body of code is evolving from a pure C base. Jeff
Jeff Law <law@redhat.com> wrote: >On 09/18/2013 11:17 AM, Michael Matz wrote: >> Hi, >> >> On Wed, 18 Sep 2013, Jeff Law wrote: >> >>> On 09/18/2013 10:24 AM, Michael Matz wrote: >>>> >>>> I'm irritated by the member name uglification (e.g. equiv_stack_ >with >>>> trailing underscore). I know that's a certain style to mark >private >>>> members, but I think it's a bad style (like prefixing variable >names with >>>> their type), and before it sets a precedent in GCCs c++ coding >style I'd >>>> like this to be changed, like in the below. >>> >>> We're already using the trailing underscore idiom for private >objects >>> moving into classes (see the pass class). >> >> I know, and I don't like it there either. >Well, as Ian pointed out, it is in our recommended style guidelines and > >you'll find uses in the vec class as well. It's well established at >this point and I see no compelling reason to go back unless you can >convince the project as a whole to change the C++ guidelines. > >> >>>> I'd also like us to not use member privatization in our classes, >but >>>> that's not in the patch, but if we could agree on that it would be >nice. >>> Member privatization is quite natural. What specifically do you not >like >>> about the practice? >> >> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00302.html >> >> That was conditional on "when we need to jump through hoops", but for >> constistency it'd make sense to avoid it everywhere. >> (I know that Ian agreed to that mail, but somehow the mailing list >> archives don't have that!?) >I don't see anything in Trevor's work that requires jumping through >hoops. It's pretty simple stuff. And again, as Ian pointed out, our >established guidelines for C++ usage encourage this behaviour. > > >> >>>> Regstrapped on x86-64-linux, okay? >>> >>> Obviously any ChangeLog, formatting and such can go in. However, >the >>> trailing underscore should stay given it's already established >practice >>> and has several nice benefits. >> >> What's the benefit of reading and writing such noisy lines? : >> >> *out_mode = mode_; >> mode_ = GET_MODE_WIDER_MODE (mode_); >> count_++; >It makes it very clear to the reader that we're dealing with objects >that belong to a class instance rather than direct access to an auto or > >static. That can be important. > There is a language specific way, too. Just qualify accesses with this-> that also avoids all the interesting name-lookup issues with dependent names. Richard. >> >> The uglification merely makes code harder to write and read, it >should be >> used in cases where you _don't_ want developers to write such names. >I feel it makes it harder in some ways and easier in others. > >Given it's recommended by our C++ guidelines which were discussed at >length, I'm going to explicitly NAK your patch. If you want to re-open > >the guidelines for C++ usage, then that's fine with me and if we as a >project change the guidelines to disallow such things, then that will >be >the time to remove the trailing underscores, private members, etc. > >FWIW, I have worked on large C++ codebases that were a free-for-all and > >found them *amazingly* painful. The restricted set allowed for GCC is >actually quite reasonable IMHO, particularly for projects where the >main >body of code is evolving from a pure C base. > > >Jeff
Hi, On Wed, 18 Sep 2013, Jeff Law wrote: > > I know, and I don't like it there either. > > Well, as Ian pointed out, it is in our recommended style guidelines and > you'll find uses in the vec class as well. As I said, yes; I also said those were pre-existing from the C times already, so they don't support the new c++ guidelines. I do have several issues with the style guidelines, and yes, it's my fault for not having gone through the pains of trying to fight off those things last year :-/ > It's well established at this point I wouldn't call two recent examples well established, but well. > I don't see anything in Trevor's work that requires jumping through > hoops. Me neither, from that perspective it's okay. It's merely that I doubt the value of any syntactic privatization like it's implemented in C++, you can #define it away, hence the compiler can't make use of that information for code generation, and the cognitive value for the developer ("hey I shouldn't look at this member from outside") is dubious, as that probably is a general rule, no direct data member access from non-members (although I have problems with that too). And I think the fact that Trevor made one data member non-private to access it from a non-member function (move_computations_dom_walker::todo) just underlines my point: private is useless and gets in the way. > > What's the benefit of reading and writing such noisy lines? : > > > > *out_mode = mode_; > > mode_ = GET_MODE_WIDER_MODE (mode_); > > count_++; > > It makes it very clear to the reader that we're dealing with objects that > belong to a class instance rather than direct access to an auto or static. > That can be important. this->x. From the wiki it seems that was dicussed (on the wiki, not the mailing list) and rejected by Lawrence on the grounds of indroducing too long lines. I agree with that, but I don't agree that therefore members should be named foo_. > Given it's recommended by our C++ guidelines which were discussed at > length, I'm going to explicitly NAK your patch. Hmmkay. > FWIW, I have worked on large C++ codebases Me too. > that were a free-for-all and found them *amazingly* painful. I don't think any of my mails about style can be interpreted as advocating free-for-all. > The restricted set allowed for GCC is actually quite reasonable IMHO, > particularly for projects where the main body of code is evolving from a > pure C base. Funnily it's the small things that weren't much discussed (probably because they are deemed not very important) in the convention that give me a hard time, nits such as these syntactic uglifications. The larger things indeed mostly are okayish. Ciao, Michael.
On Sep 19, 2013, at 6:23 AM, Michael Matz <matz@suse.de> wrote: > Me neither, from that perspective it's okay. It's merely that I doubt the > value of any syntactic privatization like it's implemented in C++, you can > #define it away, hence the compiler can't make use of that information for > code generation, and the cognitive value for the developer ("hey I > shouldn't look at this member from outside") is dubious, as that probably > is a general rule, no direct data member access from non-members (although > I have problems with that too). If we are making engineering decisions on the basis of people being able to say #define private public, well, we are so far off into the weeds as to not be funny. ODR: --each definition of D shall consist of the same sequence of tokens; Just because you see no value in private, doesn't mean others don't. Consider this, It would not be in the language if everyone shared your view.
On Thu, Sep 19, 2013 at 03:23:21PM +0200, Michael Matz wrote: > > I don't see anything in Trevor's work that requires jumping through > > hoops. > > Me neither, from that perspective it's okay. It's merely that I doubt the > value of any syntactic privatization like it's implemented in C++, you can > #define it away, hence the compiler can't make use of that information for no, it can't make use of it if someone does something crazy like #define it away which is atleast a little tricky because of the ':'. I believe clang does infact make use of private to find unused fields (maybe it does something else, but I can't imagine what that would be). > code generation, and the cognitive value for the developer ("hey I > shouldn't look at this member from outside") is dubious, as that probably > is a general rule, no direct data member access from non-members (although > I have problems with that too). The value is that when you read code you *know* that something is only used in certain places instead of hoping that is true. > And I think the fact that Trevor made one data member non-private to > access it from a non-member function (move_computations_dom_walker::todo) > just underlines my point: private is useless and gets in the way. It certainly shows a case where that's true, but it doesn't really show that's always true. > > > What's the benefit of reading and writing such noisy lines? : > > > > > > *out_mode = mode_; > > > mode_ = GET_MODE_WIDER_MODE (mode_); > > > count_++; > > > > It makes it very clear to the reader that we're dealing with objects that > > belong to a class instance rather than direct access to an auto or static. > > That can be important. > > this->x. > > From the wiki it seems that was dicussed (on the wiki, not the mailing > list) and rejected by Lawrence on the grounds of indroducing too long > lines. I agree with that, but I don't agree that therefore members should > be named foo_. this-> also has the disadvantage that you always have to rember it, and fundimentally doesn't help you know where a member could possibly be used. Trev > > > Given it's recommended by our C++ guidelines which were discussed at > > length, I'm going to explicitly NAK your patch. > > Hmmkay. > > > FWIW, I have worked on large C++ codebases > > Me too. > > > that were a free-for-all and found them *amazingly* painful. > > I don't think any of my mails about style can be interpreted as advocating > free-for-all. > > > The restricted set allowed for GCC is actually quite reasonable IMHO, > > particularly for projects where the main body of code is evolving from a > > pure C base. > > Funnily it's the small things that weren't much discussed (probably > because they are deemed not very important) in the convention that give > me a hard time, nits such as these syntactic uglifications. The larger > things indeed mostly are okayish. > > > Ciao, > Michael.
Michael Matz <matz@suse.de> writes: > What's the benefit of reading and writing such noisy lines? : > > *out_mode = mode_; > mode_ = GET_MODE_WIDER_MODE (mode_); > count_++; > > The uglification merely makes code harder to write and read, it should be > used in cases where you _don't_ want developers to write such names. Heh. Since it's my code being used as the example here: I also find it very ugly FWIW. I only added the underscores because that's what the conventions said. But we're never going to get consensus on this kind of thing. E.g. I know some people really hate the GNU formatting style (although I very much like it). So I just held my nose while writing the patch. Thanks, Richard
On Thu, Sep 19, 2013 at 7:24 PM, Trevor Saunders <tsaunders@mozilla.com> wrote: > On Thu, Sep 19, 2013 at 03:23:21PM +0200, Michael Matz wrote: >> > I don't see anything in Trevor's work that requires jumping through >> > hoops. >> >> Me neither, from that perspective it's okay. It's merely that I doubt the >> value of any syntactic privatization like it's implemented in C++, you can >> #define it away, hence the compiler can't make use of that information for > > no, it can't make use of it if someone does something crazy like #define > it away which is atleast a little tricky because of the ':'. I believe > clang does infact make use of private to find unused fields (maybe it > does something else, but I can't imagine what that would be). > >> code generation, and the cognitive value for the developer ("hey I >> shouldn't look at this member from outside") is dubious, as that probably >> is a general rule, no direct data member access from non-members (although >> I have problems with that too). > > The value is that when you read code you *know* that something is only > used in certain places instead of hoping that is true. > >> And I think the fact that Trevor made one data member non-private to >> access it from a non-member function (move_computations_dom_walker::todo) >> just underlines my point: private is useless and gets in the way. > > It certainly shows a case where that's true, but it doesn't really show > that's always true. > >> > > What's the benefit of reading and writing such noisy lines? : >> > > >> > > *out_mode = mode_; >> > > mode_ = GET_MODE_WIDER_MODE (mode_); >> > > count_++; >> > >> > It makes it very clear to the reader that we're dealing with objects that >> > belong to a class instance rather than direct access to an auto or static. >> > That can be important. >> >> this->x. >> >> From the wiki it seems that was dicussed (on the wiki, not the mailing >> list) and rejected by Lawrence on the grounds of indroducing too long >> lines. I agree with that, but I don't agree that therefore members should >> be named foo_. > > this-> also has the disadvantage that you always have to rember it, and > fundimentally doesn't help you know where a member could possibly be > used. Sure, there is no way to syntactically force the use of this-> - that's a disadvantage (though we've had private -Wxxx warnings which we could add for this). The extra advantage of this-> is that it makes name-lookup unambiguous to those not 100% familiar with it (and who really is ...) Richard. > Trev > >> >> > Given it's recommended by our C++ guidelines which were discussed at >> > length, I'm going to explicitly NAK your patch. >> >> Hmmkay. >> >> > FWIW, I have worked on large C++ codebases >> >> Me too. >> >> > that were a free-for-all and found them *amazingly* painful. >> >> I don't think any of my mails about style can be interpreted as advocating >> free-for-all. >> >> > The restricted set allowed for GCC is actually quite reasonable IMHO, >> > particularly for projects where the main body of code is evolving from a >> > pure C base. >> >> Funnily it's the small things that weren't much discussed (probably >> because they are deemed not very important) in the convention that give >> me a hard time, nits such as these syntactic uglifications. The larger >> things indeed mostly are okayish. >> >> >> Ciao, >> Michael.
On Thu, Sep 19, 2013 at 8:11 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Michael Matz <matz@suse.de> writes: >> What's the benefit of reading and writing such noisy lines? : >> >> *out_mode = mode_; >> mode_ = GET_MODE_WIDER_MODE (mode_); >> count_++; >> >> The uglification merely makes code harder to write and read, it should be >> used in cases where you _don't_ want developers to write such names. > > Heh. Since it's my code being used as the example here: I also find it > very ugly FWIW. I only added the underscores because that's what the > conventions said. > > But we're never going to get consensus on this kind of thing. E.g. I > know some people really hate the GNU formatting style (although I very > much like it). So I just held my nose while writing the patch. Btw, I've come around multiple coding-styles in the past and I definitely would prefer m_mode / m_count to mark members vs. mode_ and count_. (and s_XXX for static members IIRC). Richard. > Thanks, > Richard
On Fri, Sep 20, 2013 at 10:13:27AM +0200, Richard Biener wrote: > On Thu, Sep 19, 2013 at 8:11 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: > > Michael Matz <matz@suse.de> writes: > >> What's the benefit of reading and writing such noisy lines? : > >> > >> *out_mode = mode_; > >> mode_ = GET_MODE_WIDER_MODE (mode_); > >> count_++; > >> > >> The uglification merely makes code harder to write and read, it should be > >> used in cases where you _don't_ want developers to write such names. > > > > Heh. Since it's my code being used as the example here: I also find it > > very ugly FWIW. I only added the underscores because that's what the > > conventions said. > > > > But we're never going to get consensus on this kind of thing. E.g. I > > know some people really hate the GNU formatting style (although I very > > much like it). So I just held my nose while writing the patch. > > Btw, I've come around multiple coding-styles in the past and I definitely > would prefer m_mode / m_count to mark members vs. mode_ and count_. > (and s_XXX for static members IIRC). what about a_foo for arguments? I'd prefer m_/s_foo for members / static things too fwiw. Trev > > Richard. > > > Thanks, > > Richard
Hi, On Fri, 20 Sep 2013, Trevor Saunders wrote: > > > very ugly FWIW. I only added the underscores because that's what > > > the conventions said. > > > > > > But we're never going to get consensus on this kind of thing. E.g. > > > I know some people really hate the GNU formatting style (although I > > > very much like it). So I just held my nose while writing the patch. > > > > Btw, I've come around multiple coding-styles in the past and I > > definitely would prefer m_mode / m_count to mark members vs. mode_ and > > count_. (and s_XXX for static members IIRC). > > what about a_foo for arguments? That would go too far. If we're marking member for reasons of reminding ourself that the access involves an indirection (amongst other reasons), i.e. it's slower than accessing a local variable, then the same can't be said from arguments. It's just another local variable mostly. And about remembering that it's in current scope, well, it's just there at the very top of the function. (Or IOW, we could do without the last 20 years :) ) > I'd prefer m_/s_foo for members / static things too fwiw. Me as well. It's still ugly, but not so unsymmetric as the trailing underscore. Ciao, Michael.
On 09/20/2013 02:13 AM, Richard Biener wrote: > On Thu, Sep 19, 2013 at 8:11 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Michael Matz <matz@suse.de> writes: >>> What's the benefit of reading and writing such noisy lines? : >>> >>> *out_mode = mode_; >>> mode_ = GET_MODE_WIDER_MODE (mode_); >>> count_++; >>> >>> The uglification merely makes code harder to write and read, it should be >>> used in cases where you _don't_ want developers to write such names. >> >> Heh. Since it's my code being used as the example here: I also find it >> very ugly FWIW. I only added the underscores because that's what the >> conventions said. >> >> But we're never going to get consensus on this kind of thing. E.g. I >> know some people really hate the GNU formatting style (although I very >> much like it). So I just held my nose while writing the patch. > > Btw, I've come around multiple coding-styles in the past and I definitely > would prefer m_mode / m_count to mark members vs. mode_ and count_. > (and s_XXX for static members IIRC). That would be fine with me. If you want to propose this as a change to our coding standards, I'd support it. Then it's just a matter of changing the existing code.. Sigh.. jeff
Index: domwalk.h =================================================================== --- domwalk.h (revision 202698) +++ domwalk.h (working copy) @@ -21,16 +21,14 @@ along with GCC; see the file COPYING3. #ifndef GCC_DOM_WALK_H #define GCC_DOM_WALK_H -/** - * This is the main class for the dominator walker. It is expected that - * consumers will have a custom class inheriting from it, which will over ride - * at least one of before_dom_children and after_dom_children to implement the - * custom behavior. - */ +/* This is the main class for the dominator walker. It is expected that + consumers will have a custom class inheriting from it, which will over ride + at least one of before_dom_children and after_dom_children to implement the + custom behavior. */ class dom_walker { public: - dom_walker (cdi_direction direction) : dom_direction_ (direction) {} + dom_walker (cdi_direction direction) : dom_direction (direction) {} /* Walk the dominator tree. */ void walk (basic_block); @@ -46,7 +44,7 @@ private: if it is set to CDI_DOMINATORS, then we walk the dominator tree, if it is set to CDI_POST_DOMINATORS, then we walk the post dominator tree. */ - const ENUM_BITFIELD (cdi_direction) dom_direction_ : 2; + const ENUM_BITFIELD (cdi_direction) dom_direction : 2; }; #endif Index: domwalk.c =================================================================== --- domwalk.c (revision 202698) +++ domwalk.c (working copy) @@ -154,7 +154,7 @@ dom_walker::walk (basic_block bb) int sp = 0; int *postorder, postorder_num; - if (dom_direction_ == CDI_DOMINATORS) + if (dom_direction == CDI_DOMINATORS) { postorder = XNEWVEC (int, n_basic_blocks); postorder_num = inverted_post_order_compute (postorder); @@ -181,10 +181,10 @@ dom_walker::walk (basic_block bb) worklist[sp++] = NULL; int saved_sp = sp; - for (dest = first_dom_son (dom_direction_, bb); - dest; dest = next_dom_son (dom_direction_, dest)) + for (dest = first_dom_son (dom_direction, bb); + dest; dest = next_dom_son (dom_direction, dest)) worklist[sp++] = dest; - if (dom_direction_ == CDI_DOMINATORS) + if (dom_direction == CDI_DOMINATORS) switch (sp - saved_sp) { case 0: @@ -210,7 +210,7 @@ dom_walker::walk (basic_block bb) else break; } - if (dom_direction_ == CDI_DOMINATORS) + if (dom_direction == CDI_DOMINATORS) { free (bb_postorder); bb_postorder = NULL; Index: graphite-sese-to-poly.c =================================================================== --- graphite-sese-to-poly.c (revision 202698) +++ graphite-sese-to-poly.c (working copy) @@ -1226,21 +1226,21 @@ public: virtual void after_dom_children (basic_block); private: - vec<gimple> conditions_, cases_; - sese region_; + vec<gimple> conditions, cases; + sese region; }; sese_dom_walker::sese_dom_walker (cdi_direction direction, sese region) - : dom_walker (direction), region_ (region) + : dom_walker (direction), region (region) { - conditions_.create (3); - cases_.create (3); + conditions.create (3); + cases.create (3); } sese_dom_walker::~sese_dom_walker () { - conditions_.release (); - cases_.release (); + conditions.release (); + cases.release (); } /* Call-back for dom_walk executed before visiting the dominated @@ -1252,7 +1252,7 @@ sese_dom_walker::before_dom_children (ba gimple_bb_p gbb; gimple stmt; - if (!bb_in_sese_p (bb, region_)) + if (!bb_in_sese_p (bb, region)) return; stmt = single_pred_cond_non_loop_exit (bb); @@ -1261,20 +1261,20 @@ sese_dom_walker::before_dom_children (ba { edge e = single_pred_edge (bb); - conditions_.safe_push (stmt); + conditions.safe_push (stmt); if (e->flags & EDGE_TRUE_VALUE) - cases_.safe_push (stmt); + cases.safe_push (stmt); else - cases_.safe_push (NULL); + cases.safe_push (NULL); } gbb = gbb_from_bb (bb); if (gbb) { - GBB_CONDITIONS (gbb) = conditions_.copy (); - GBB_CONDITION_CASES (gbb) = cases_.copy (); + GBB_CONDITIONS (gbb) = conditions.copy (); + GBB_CONDITION_CASES (gbb) = cases.copy (); } } @@ -1284,13 +1284,13 @@ sese_dom_walker::before_dom_children (ba void sese_dom_walker::after_dom_children (basic_block bb) { - if (!bb_in_sese_p (bb, region_)) + if (!bb_in_sese_p (bb, region)) return; if (single_pred_cond_non_loop_exit (bb)) { - conditions_.pop (); - cases_.pop (); + conditions.pop (); + cases.pop (); } } Index: tree-into-ssa.c =================================================================== --- tree-into-ssa.c (revision 202698) +++ tree-into-ssa.c (working copy) @@ -2075,7 +2075,8 @@ rewrite_update_phi_arguments (basic_bloc class rewrite_update_dom_walker : public dom_walker { public: - rewrite_update_dom_walker (cdi_direction direction) : dom_walker (direction) {} + rewrite_update_dom_walker (cdi_direction direction) : dom_walker (direction) + {} virtual void before_dom_children (basic_block); virtual void after_dom_children (basic_block); @@ -2235,17 +2236,17 @@ private: /* Notice that this bitmap is indexed using variable UIDs, so it must be large enough to accommodate all the variables referenced in the function, not just the ones we are renaming. */ - bitmap kills_; + bitmap kills; }; mark_def_dom_walker::mark_def_dom_walker (cdi_direction direction) - : dom_walker (direction), kills_ (BITMAP_ALLOC (NULL)) + : dom_walker (direction), kills (BITMAP_ALLOC (NULL)) { } mark_def_dom_walker::~mark_def_dom_walker () { - BITMAP_FREE (kills_); + BITMAP_FREE (kills); } /* Block processing routine for mark_def_sites. Clear the KILLS bitmap @@ -2256,9 +2257,9 @@ mark_def_dom_walker::before_dom_children { gimple_stmt_iterator gsi; - bitmap_clear (kills_); + bitmap_clear (kills); for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) - mark_def_sites (bb, gsi_stmt (gsi), kills_); + mark_def_sites (bb, gsi_stmt (gsi), kills); } /* Initialize internal data needed during renaming. */ Index: tree-ssa-dom.c =================================================================== --- tree-ssa-dom.c (revision 202698) +++ tree-ssa-dom.c (working copy) @@ -774,7 +774,7 @@ class dom_opt_dom_walker : public dom_wa { public: dom_opt_dom_walker (cdi_direction direction) - : dom_walker (direction), dummy_cond_ (NULL) {} + : dom_walker (direction), dummy_cond (NULL) {} virtual void before_dom_children (basic_block); virtual void after_dom_children (basic_block); @@ -782,7 +782,7 @@ public: private: void thread_across_edge (edge); - gimple dummy_cond_; + gimple dummy_cond; }; /* Jump threading, redundancy elimination and const/copy propagation. @@ -1077,13 +1077,13 @@ simplify_stmt_for_jump_threading (gimple void dom_opt_dom_walker::thread_across_edge (edge e) { - if (! dummy_cond_) - dummy_cond_ = + if (! dummy_cond) + dummy_cond = gimple_build_cond (NE_EXPR, integer_zero_node, integer_zero_node, NULL, NULL); - ::thread_across_edge (dummy_cond_, e, false, + ::thread_across_edge (dummy_cond, e, false, &const_and_copies_stack, simplify_stmt_for_jump_threading); } Index: tree-ssa-loop-im.c =================================================================== --- tree-ssa-loop-im.c (revision 202698) +++ tree-ssa-loop-im.c (working copy) @@ -1192,11 +1192,11 @@ class move_computations_dom_walker : pub { public: move_computations_dom_walker (cdi_direction direction) - : dom_walker (direction), todo_ (0) {} + : dom_walker (direction), todo (0) {} virtual void before_dom_children (basic_block); - unsigned int todo_; + unsigned int todo; }; /* Hoist the statements in basic block BB out of the loops prescribed by @@ -1268,7 +1268,7 @@ move_computations_dom_walker::before_dom gimple_phi_result (stmt), t, arg0, arg1); SSA_NAME_DEF_STMT (gimple_phi_result (stmt)) = new_stmt; - todo_ |= TODO_cleanup_cfg; + todo |= TODO_cleanup_cfg; } gsi_insert_on_edge (loop_preheader_edge (level), new_stmt); remove_phi_node (&bsi, false); @@ -1346,7 +1346,7 @@ move_computations (void) if (need_ssa_update_p (cfun)) rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa); - return walker.todo_; + return walker.todo; } /* Checks whether the statement defining variable *INDEX can be hoisted Index: tree-ssa-phiopt.c =================================================================== --- tree-ssa-phiopt.c (revision 202698) +++ tree-ssa-phiopt.c (working copy) @@ -1379,13 +1379,13 @@ class nontrapping_dom_walker : public do { public: nontrapping_dom_walker (cdi_direction direction, pointer_set_t *ps) - : dom_walker (direction), nontrapping_ (ps) {} + : dom_walker (direction), nontrapping (ps) {} virtual void before_dom_children (basic_block); virtual void after_dom_children (basic_block); private: - pointer_set_t *nontrapping_; + pointer_set_t *nontrapping; }; /* Called by walk_dominator_tree, when entering the block BB. */ @@ -1416,8 +1416,8 @@ nontrapping_dom_walker::before_dom_child nt_call_phase++; else if (gimple_assign_single_p (stmt) && !gimple_has_volatile_ops (stmt)) { - add_or_mark_expr (bb, gimple_assign_lhs (stmt), nontrapping_, true); - add_or_mark_expr (bb, gimple_assign_rhs1 (stmt), nontrapping_, false); + add_or_mark_expr (bb, gimple_assign_lhs (stmt), nontrapping, true); + add_or_mark_expr (bb, gimple_assign_rhs1 (stmt), nontrapping, false); } } } Index: tree-ssa-uncprop.c =================================================================== --- tree-ssa-uncprop.c (revision 202698) +++ tree-ssa-uncprop.c (working copy) @@ -360,11 +360,11 @@ public: uncprop_dom_walker (cdi_direction direction) : dom_walker (direction) { - equiv_stack_.create (2); + equiv_stack.create (2); } ~uncprop_dom_walker () { - equiv_stack_.release (); + equiv_stack.release (); } virtual void before_dom_children (basic_block); @@ -376,7 +376,7 @@ private: leading to this block. If no such edge equivalency exists, then we record NULL. These equivalences are live until we leave the dominator subtree rooted at the block where we record the equivalency. */ - vec<tree> equiv_stack_; + vec<tree> equiv_stack; }; /* Main driver for un-cprop. */ @@ -428,7 +428,7 @@ void uncprop_dom_walker::after_dom_children (basic_block bb ATTRIBUTE_UNUSED) { /* Pop the topmost value off the equiv stack. */ - tree value = equiv_stack_.pop (); + tree value = equiv_stack.pop (); /* If that value was non-null, then pop the topmost equivalency off its equivalency stack. */ @@ -566,13 +566,13 @@ uncprop_dom_walker::before_dom_children struct edge_equivalency *equiv = (struct edge_equivalency *) e->aux; record_equiv (equiv->rhs, equiv->lhs); - equiv_stack_.safe_push (equiv->rhs); + equiv_stack.safe_push (equiv->rhs); recorded = true; } } if (!recorded) - equiv_stack_.safe_push (NULL_TREE); + equiv_stack.safe_push (NULL_TREE); uncprop_into_successor_phis (bb); } Index: ChangeLog =================================================================== --- ChangeLog (revision 202698) +++ ChangeLog (working copy) @@ -91,28 +91,29 @@ (move_computations_stmt): Convert to method move_computations_dom_walker::before_dom_children. (move_computations, tree_ssa_lim): Adjust. - * tree-ssa-phiopt.c (nontrapping_dom_walker): new class - (nt_init_block): Make method + * tree-ssa-phiopt.c (nontrapping_dom_walker): New class. + (nt_init_block): Convert to method notrappping_dom_walker::before_dom_children. - (nt_fini_block): Make + (nt_fini_block): Convert to method method nontrapping_dom_walker::after_dom_children. (get_non_trapping): Adjust. * tree-ssa-pre.c (eliminate_dom_walker): New class. - (eliminate_bb): Make method eliminate_dom_walker::before_dom_children. - (eliminate_leave_block): Make method. + (eliminate_bb): Convert to method + eliminate_dom_walker::before_dom_children. + (eliminate_leave_block): Convert to method eliminate_dom_walker::after_dom_children. - (eliminate): Adjust + (eliminate): Adjust. * tree-ssa-strlen.c (strlen_dom_walker): New class. - (strlen_enter_block): Make method + (strlen_enter_block): Convert to method strlen_dom_walker::before_dom_children. - (strlen_leave_block): Make + (strlen_leave_block): Convert to method method strlen_dom_walker::after_dom_children. (tree_ssa_strlen): Adjust. * tree-ssa-uncprop.c (uncprop_dom_walker): New class. (tree_ssa_uncprop): Adjust. - (uncprop_leave_block): Make method + (uncprop_leave_block): Convert to method uncprop_dom_walker::after_dom_children. - (uncprop_leave_block): Make method + (uncprop_leave_block): Convert to method uncprop_dom_walker::before_dom_children. 2013-09-18 Bin Cheng <bin.cheng@arm.com>