2009/4/15 <[email protected]>: > LG. I hope we have good coverage of the generated code. You should try > sprinkling __ stop(...) at various places and make sure you hit them -
That turned out to be rather tedious, so I added some stuff for autogenerating a coverage report. It's now mixed up in this change list. I added tests for the stuff related to this change that was not covered by our mjsunit tests. > if not we need more tests. Does it lint? It did lint and it still does. > http://codereview.chromium.org/67163/diff/1/14 > File src/assembler.cc (right): > > http://codereview.chromium.org/67163/diff/1/14#newcode613 > Line 613: break; > How about a default clause that triggers an UNREACHABLE? Done > http://codereview.chromium.org/67163/diff/1/10 > File src/assembler.h (right): > > http://codereview.chromium.org/67163/diff/1/10#newcode420 > Line 420: static ExternalReference double_fp_operation(char operation); > Why is the operation a char? Why not a Token::Value or something like Done. > that? ... and it seems a dodgy to have this on IA32 where we don't need > it at all. I tried various other places and they all required disproportionately large restructuring, so I ended up leaving them there. They are tiny. > http://codereview.chromium.org/67163/diff/1/11 > File src/codegen-arm.cc (right): > > http://codereview.chromium.org/67163/diff/1/11#newcode4516 > Line 4516: char operation, > Token::Value operation? Done > http://codereview.chromium.org/67163/diff/1/11#newcode4543 > Line 4543: // Get type of r1 into r3 > Terminate comment with . Done. > http://codereview.chromium.org/67163/diff/1/6 > File src/codegen-ia32.h (right): > > http://codereview.chromium.org/67163/diff/1/6#newcode435 > Line 435: const OverwriteMode overwrite_mode); > I wonder why this is const? Gone. > http://codereview.chromium.org/67163/diff/1/9 > File src/simulator-arm.cc (right): > > http://codereview.chromium.org/67163/diff/1/9#newcode433 > Line 433: int sixty_four_bits[2]; > Shouldn't this be int32 or something like that? Done. > http://codereview.chromium.org/67163/diff/1/9#newcode447 > Line 447: int sixty_four_bits[2]; > int32? Done > http://codereview.chromium.org/67163/diff/1/9#newcode458 > Line 458: registers_[2] = 0x50Bad4U; > We have a recognizable zap value you could use here: kZapValue. It's not really accessible in the simulator and it has the wrong type. I left the new value there. I prefer different values anyway since they make it easier to find out where the data was zapped. > http://codereview.chromium.org/67163/diff/1/9#newcode937 > Line 937: double z = (swi == simulator_fp_add) ? > This looks nasty. Why not write it as a series of if-else-if? Rewritten as separate cases. > http://codereview.chromium.org/67163/diff/1/3 > File test/mjsunit/number-limits.js (right): > > http://codereview.chromium.org/67163/diff/1/3#newcode36 > Line 36: print("i = " + i + ", 1/eps = " + (1/eps) + ", mulAboveMax = " > + mulAboveMax + ", Number.MAX_VALUE = " + Number.MAX_VALUE + ", Infinity > = " + Infinity); > Lines too long. Fixed. -- Erik Corry, Software Engineer Google Denmark ApS. CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
