In the interest of getting Release 5.0 out, allow me to request the following for this patch:
1. I really want this patch to be part of the 5.0 Release. 2. Yongchong, this fix looks good. Also, in our private e-mail exchanges, you indicated to me that this patch passed all regression and application testing, so unless someone objects, please check this patch into the trunk. This will be the last patch to be included in the 5.0 branch. 3. Jian-Xin, you have proposed that instead of doing memcmp, compare st1 and st2 field by field. Yongchong, please consider/discuss his proposal after your check-in. (In other words, I suggest that we defer Jian-Xin's proposal until *post* 5.0 Release.) Thanks, Michael Lai [PS: I am working on the 5.0 branch as we speak. Wait for my next e-mail.] -----Original Message----- From: Wu Yongchong [mailto:wuyongch...@gmail.com] Sent: Sunday, October 30, 2011 7:47 PM To: Sun Chan Cc: open64-devel Subject: Re: [Open64-devel] [open64-devel] Code review request for bug 883 [IPA] Hi, what about this, I change the position of the comment to top of the class member definition Index: osprey/common/com/symtab_defs.h =================================================================== --- osprey/common/com/symtab_defs.h (revision 3775) +++ osprey/common/com/symtab_defs.h (working copy) @@ -223,6 +223,8 @@ class ST { public: + // after add new member, Make sure to update function eq_const_st::operator() + // in file ipc_symtab_merge.cxx union { STR_IDX name_idx; // index to the name string TCON_IDX tcon; // constant value Index: osprey/ipa/common/ipc_symtab_merge.cxx =================================================================== --- osprey/ipa/common/ipc_symtab_merge.cxx (revision 3775) +++ osprey/ipa/common/ipc_symtab_merge.cxx (working copy) @@ -159,7 +159,7 @@ struct eq_const_st { bool operator() (const ST* st1, const ST* st2) const { - return memcmp (st1, st2, sizeof(ST) - sizeof(ST_IDX) * 2 - sizeof(TY_IDX)) == 0; + return memcmp (st1, st2, sizeof(ST) - sizeof(ST_IDX) * 2 - sizeof(TY_IDX) - sizeof(mUINT32)) == 0; } }; On Mon, Oct 31, 2011 at 7:09 AM, Sun Chan <sun.c...@gmail.com> wrote: > your comment should be at where future new member may be added. > sun > > On Sun, Oct 30, 2011 at 11:01 PM, Wu Yongchong <wuyongch...@gmail.com> > wrote: >> >> Hi, all >> Can a gate keeper help review this patch. >> See https://bugs.open64.net/show_bug.cgi?id=883 for reference. >> Revision 3760 add a new member "mINT32 line" to structure ST, it >> cause the function eq_const_st::operator() get wrong return value, >> then make the symbol merge process in IPA not work. following is the >> patch. I add a common to make sure anyone who add new member to >> structure ST also update function eq_const_st::operator() >> >> Index: osprey/common/com/symtab_defs.h >> =================================================================== >> --- osprey/common/com/symtab_defs.h (revision 3776) >> +++ osprey/common/com/symtab_defs.h (working copy) >> @@ -261,6 +261,8 @@ >> >> mUINT32 line; // The line num where define the sym in >> the source file. >> >> + // Make sure to update function eq_const_st::operator() in file >> ipc_symtab_merge.cxx >> + >> // operations >> >> ST () {Fail_FmtAssertion("ST default constructor must not be >> called.");} >> Index: osprey/ipa/common/ipc_symtab_merge.cxx >> =================================================================== >> --- osprey/ipa/common/ipc_symtab_merge.cxx (revision 3776) >> +++ osprey/ipa/common/ipc_symtab_merge.cxx (working copy) >> @@ -159,7 +159,7 @@ >> struct eq_const_st >> { >> bool operator() (const ST* st1, const ST* st2) const { >> - return memcmp (st1, st2, sizeof(ST) - sizeof(ST_IDX) * 2 - >> sizeof(TY_IDX)) == 0; >> + return memcmp (st1, st2, sizeof(ST) - sizeof(ST_IDX) * 2 - >> sizeof(TY_IDX) - sizeof(mUINT32)) == 0; >> } >> }; >> >> -- >> yongchong >> >> >> --------------------------------------------------------------------- >> --------- Get your Android app more play: Bring it to the BlackBerry >> PlayBook in minutes. BlackBerry App World™ now supports >> Android™ Apps for the BlackBerry® PlayBook™. Discover >> just how easy and simple it is! http://p.sf.net/sfu/android-dev2dev >> _______________________________________________ >> Open64-devel mailing list >> Open64-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/open64-devel > > -- yongchong ------------------------------------------------------------------------------ Get your Android app more play: Bring it to the BlackBerry PlayBook in minutes. BlackBerry App World™ now supports Android™ Apps for the BlackBerry® PlayBook™. Discover just how easy and simple it is! http://p.sf.net/sfu/android-dev2dev _______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel ------------------------------------------------------------------------------ Get your Android app more play: Bring it to the BlackBerry PlayBook in minutes. BlackBerry App World™ now supports Android™ Apps for the BlackBerry® PlayBook™. Discover just how easy and simple it is! http://p.sf.net/sfu/android-dev2dev _______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel
