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
