JSC shell shouldn't treat NUL as a terminator when printing a JS string
Created attachment 395525 [details] Patch
Hmm, I suppose this would be the right opportunity to remove $vm.print too -- it's hardly used, and it doesn't do what it says.
(In reply to Ross Kirsling from comment #2) > Hmm, I suppose this would be the right opportunity to remove $vm.print too > -- it's hardly used, and it doesn't do what it says. Do not remove $vm.print(). Its uses go beyond what it in the existing test scripts. I use it for custom debugging tests all the time. It comes in especially handy when I want to print from WebCore and don't want to go thru console.log().
(In reply to Mark Lam from comment #3) > (In reply to Ross Kirsling from comment #2) > > Hmm, I suppose this would be the right opportunity to remove $vm.print too > > -- it's hardly used, and it doesn't do what it says. > > Do not remove $vm.print(). Its uses go beyond what it in the existing test > scripts. I use it for custom debugging tests all the time. It comes in > especially handy when I want to print from WebCore and don't want to go thru > console.log(). Ah okay, thanks for the clarification. My concern is just because $vm.print is currently effectively dataLogLn, and I'm not sure whether this same change is appropriate to apply to dataLog.
Why should the jsc shell treat NULL as a terminator when printing a JS string? JS strings come with a length, and is not necessarily null terminated like a C string. I fail to see why we would want to make this change. In fact, it feels wrong to do so.
(In reply to Mark Lam from comment #5) > Why should the jsc shell treat NULL as a terminator when printing a JS > string? JS strings come with a length, and is not necessarily null > terminated like a C string. I fail to see why we would want to make this > change. In fact, it feels wrong to do so. I talked with Ross offline and clarified things. I was mistaken above and misread the change as the opposite of what it's doing. Sorry for that. Carry on. No need to worry about $vm.print() because it is purely a WebKit developer debugging tool, not a user facing tool. User (as in the developer) beware when using it.
Created attachment 395533 [details] Patch
Comment on attachment 395533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395533&action=review > Source/JavaScriptCore/jsc.cpp:1293 > + fprintf(stderr, "--> "); fputs? > Source/JavaScriptCore/jsc.cpp:1295 > + fprintf(stderr, "\n"); putc? > Source/JavaScriptCore/jsc.cpp:2803 > + printf("Exception: "); puts? > Source/JavaScriptCore/jsc.cpp:2808 > + printf("\n"); putchar? > JSTests/ChangeLog:11 > + Update baseline, but ensure that \0 isn't printed to file, lest the file be treated as binary. For testing purposes I often like the idea of replacing a NUL with something like "<NUL>" rather than just deleting it. Seems more likely to help us catch bugs and unlikely to cause confusion.
(In reply to Darin Adler from comment #8) > > JSTests/ChangeLog:11 > > + Update baseline, but ensure that \0 isn't printed to file, lest the file be treated as binary. > > For testing purposes I often like the idea of replacing a NUL with something > like "<NUL>" rather than just deleting it. Seems more likely to help us > catch bugs and unlikely to cause confusion. That's a good idea, though it's probably inappropriate that I edit this test in the first place. :-/ Given that JSTests/ChakraCore is the only subdirectory that does file diffs and only one of its tests is having this problem, it might be simpler to just map its echo function to $vm.print instead...
(In reply to Ross Kirsling from comment #9) > Given that JSTests/ChakraCore is the only subdirectory that does file diffs > and only one of its tests is having this problem, it might be simpler to > just map its echo function to $vm.print instead... Hrmm, but apparently $vm.print doesn't insert spaces between its arguments, so we'd need to either make it do so or update two dozen baselines. That or I suppose we could just have .baseline-jsc _be_ binary for this test. It's gross, but the data is correct, and we'd avoid modifying files we shouldn't.
Created attachment 395540 [details] Patch for landing
(In reply to Ross Kirsling from comment #10) > That or I suppose we could just have .baseline-jsc _be_ binary for this > test. It's gross, but the data is correct, and we'd avoid modifying files we > shouldn't. Seems like this works fine after all, so long as the file is marked diffable in .gitattributes.
Found 1 new test failure: storage/indexeddb/value-cursor-cycle.html
Committed r259564: <https://trac.webkit.org/changeset/259564> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395540 [details].
<rdar://problem/61328543>