Hi Eric, Do you think you would have time to take a quick look if it looks ok after I updated according to your comments?
(Sorry that I mail you again, I just wanted to make sure you did not miss the previous mail.) Best Regards, Andreas 2010/9/8 <[email protected]> > Eric, > Uploaded new snapshot after I fixed your comments. > Rodolph and Søren, I hope you are still satisfied after these new (minor) > changes. > > > http://codereview.chromium.org/3341012/diff/9001/10002 > > File src/arm/code-stubs-arm.cc (right): > > http://codereview.chromium.org/3341012/diff/9001/10002#newcode4146 > src/arm/code-stubs-arm.cc:4146: // r6 = to (smi), r7 = from (smi) > On 2010/09/08 07:14:03, Søren Gjesse wrote: > >> Just a suggestion, but as r6 and r7 hold the same values throughout >> > this > >> function using variables for the to and from registers >> > > Register to = r6 >> Register from = r7 >> > > will make things easier to read, and you can avoid having to hassle >> > with keeping > >> comments in sync with the code. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002 > File src/arm/code-stubs-arm.cc (left): > > http://codereview.chromium.org/3341012/diff/14001/15002#oldcode4142 > src/arm/code-stubs-arm.cc:4142: static const int kFromOffset = 1 * > kPointerSize; > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Please leave this constant there because it serves to document the >> > layout on the > >> stack. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002 > File src/arm/code-stubs-arm.cc (right): > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4149 > src/arm/code-stubs-arm.cc:4149: __ Ldrd(to, from, MemOperand(sp, > kToOffset)); > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Here you should statically assert that the FromOffset is the ToOffset >> > + 4. That > >> way if someone tries to rearrange things they will get an error. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4234 > src/arm/code-stubs-arm.cc:4234: masm, r3, r4, r1, r5, to, from, r9, > &make_two_character_string); > On 2010/09/08 11:02:54, Erik Corry wrote: > >> To and from are just being used as scratch registers here. Therefore >> > it is > >> misleading to use their names. I would rather just use r6 and r7 on >> > this line. > > > Done. > (I had the same thought from the beginning, but then I thought it was > better to be consistent and not mix r6, r7, 'to' and 'from' in this > function. That way it would be more obvious that it is actually 'to' and > 'from' that was used as scratch registers. The reader of the code would > only need to keep track of either r6 and r7 or 'to' and 'from', not > both. I thought that you anyway need to look at the prototype for > GenerateTwoCharacterSymbolTableProbe() to know which registers are > scratch registers. Anyway I changed according your comment now, since I > also see your point.) > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4267 > src/arm/code-stubs-arm.cc:4267: > StringHelper::GenerateCopyCharactersLong(masm, r1, r5, r2, r3, r4, to, > from, > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Likewise here. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4268 > src/arm/code-stubs-arm.cc:4268: r9, COPY_ASCII | DEST_ALWAYS_ALIGNED); > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Indentation is wrong here now. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4276 > src/arm/code-stubs-arm.cc:4276: // Check for flat two byte string. > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Since the value in the 'from' register is used lower down it is still >> > live. It > >> is problematic to remove the comment here and similar ones above since >> > they > >> documented that fact. >> > > Done. > I added this comment again and the ones above where r6 or r7 where used > further down. Please tell me if you wanted me to keep all the comments > for the registers. I did not interpret your comment that way. So I only > put them in when r6 or r7 where used in the code below the comment. > Actually I interpreted Søren's comment as if he wanted me to remove the > comments when I replaced r6 with 'to' and r7 with 'from'. Anyway I see > your point and hope I have changed according your comment. > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4297 > src/arm/code-stubs-arm.cc:4297: > StringHelper::GenerateCopyCharactersLong(masm, r1, r5, r2, r3, r4, to, > from, > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Again, these are just scratch registers. The names 'from' and 'to' >> > are > >> misleading. >> > > Done. > > > http://codereview.chromium.org/3341012/diff/14001/15002#newcode4374 > src/arm/code-stubs-arm.cc:4374: __ Ldrd(r0 , r1, MemOperand(sp)); // > Load right in r0, left in r1 > On 2010/09/08 11:02:54, Erik Corry wrote: > >> Full stop/period at the end of comments. >> > > Done. > > > http://codereview.chromium.org/3341012/show > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
