Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
39523002:
Add harness for running Unicode.org Bidi tests. (Closed)
|
Created:
7 years, 1 month ago by eseidel Modified:
7 years, 1 month ago CC:
blink-reviews, igor.o, behdad_google, jww Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd harness for running Unicode.org Bidi tests.
This depends on having the BidiTest.txt file in your
$CWD when executing blink_platform_unittests:
http://www.unicode.org/Public/6.3.0/ucd/BidiTest.txt
checked in:
src/third_party/icu/source/test/testdata/BidiTest.txt
Unfortunately since we can't depend on base/ directly from
Blink we can't use base/path_service.h and thus can't
reliably find the checked-in BidiTest.txt file to run this
unittest on the bots. For now if BidiTest.txt is not in
$CWD it just logs an ERROR and bails without failing.
To run locally:
ninja -C out/Debug blink_platform_unittests
./out/Debug/blink_platform_unittests \
--gtest_filter=BidiResolver.BidiTest_txt
Currently this skips all the tests for the new isolate
control characters in Unicode 6.3. Even still, it looks like
we may have a bunch of corner cases in our UBA to fix.
WARNING: Skipped 418143 tests.
Ran 352098 tests: 44887 level failures, 19153 order failures.
I suspect there are a couple harness-integration issues
causing unnecessary level failures. Order failures are more
likely to represent user-observable bugs.
I'm interested in getting this checked in so we can iterate
on these failures. We should consider checking in a golden
file as well and comparing the output between runs instead of
just the gtest asserts I added. (Revisions for a later patch!)
I split the BidiTest.txt parsing code out into its own header
which hopefully we can upstream to Unicode.org since it seems
silly for everyone to write this (non-trivial) logic themselves.
We have no business maintaining this parsing code (especially
since the test format has changed between unicode versions).
There is another bidi test file from unicode.org:
http://www.unicode.org/Public/6.3.0/ucd/BidiCharacterTest.txt
But that will require another separate parser. :(
BUG=311432
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161046
Patch Set 1 #Patch Set 2 : Not completely busted #Patch Set 3 : Closer, still needs paragraph direction support to pass tests #Patch Set 4 : Split the parsing harness into its own file #Patch Set 5 : Actually seems to work. I think I'm setting the paragraph direction incorrectly still #Patch Set 6 : Cleaned up a little #Patch Set 7 : Much simpler error-reporting, I think this is actually useable #Patch Set 8 : Close to being useful #Patch Set 9 : Closer #Patch Set 10 : Fix AutoLTR support and generation of ordering values" #Patch Set 11 : Make it possible to skip tests for features we don't yet support #Patch Set 12 : Don't fail when we can't find BidiTest.txt, ready for review #Patch Set 13 : AppEngine 500s... #
Total comments: 3
Patch Set 14 : Add a comment as to why we can't use Platform::current()->unitTestSupport() #Patch Set 15 : update comments about reference implementations #
Total comments: 5
Patch Set 16 : Removed most of the stream usage and fixed header comments and license #
Total comments: 25
Patch Set 17 : Updated per jyasskin's review #
Total comments: 7
Patch Set 18 : For landing #Patch Set 19 : Fix mac build? #Patch Set 20 : Another attempt to fix mac #Patch Set 21 : #Patch Set 22 : Fix another warning #Patch Set 23 : Fix GCC build #Patch Set 24 : remove stray change #Patch Set 25 : Some day this change will land #Patch Set 26 : Avoid std::isspace to make msvc 2010 happy #
Messages
Total messages: 49 (0 generated)
I wonder if someone with actual std-library experience (newer than my last usage which was about 1999) could tell me how to write this awesomer. I looked briefly at using boost.spirit (a parser library) but I couldn't figure out how to build with boost in this target and gave up and wrote my own hodge-podge parser.
Ha! My example characters look completely wrong. Will fix and then tests might actually start passing.
I suspect the next step is to make the failure counting/understanding code more specific to this use case and make GTEST only assert that we have the expected number of failures. Right now it's very difficult to parse the failure information (or understand how many we have) from the gtest output.
FWIW here's my implementation for testing fribidi: https://github.com/googlei18n/fribidi-vs-unicode/blob/master/test.c
There are still some harness bugs, but when running the full suite, I currently get: Ran 770241 tests: 451931 level failures, 322018 order failures.
Latest patch: Ran 770241 tests: 320196 level failures, 123737 order failures.
So the major issue blocking landing this is me figuring out how this is expected to integrate with our build system. I'm not quite sure how unittest binaries are supposed to handle dependent files. I suspect I'm supposed to package up the text file into a .pak file or similar as part of the build process, or possibly install it into the data section of the resulting unittest binary. I'm just not sure. :)
In chrome land there's test data in content/test/data/ . I don't think that's usually bundled up. Here's a random file of these getting loaded: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... That unicode file is kinda large though – don't we have the same information in ICU already? Could we use that somehow?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so... is likely where it would be if we had it. A couple problems though: 1. Our version of ICU isn't quite up to date with Unicode 6.3 yet, so it wouldn't have the right version of this data, even if we had it. 2. We have our own implementation of the UBA, so although we could happily share this test data with ICU the code driven by the tests would be wholly separate. 3. I would expect ICU to have a copy of this in their repo but not to ship it as part of their distributed package: http://source.icu-project.org/repos/icu/icu/trunk/source/test/intltest/bidico... http://source.icu-project.org/repos/icu/icu/trunk/source/test/testdata/BidiTe... is there copy. So yeah, if we included icu/test in our checkout then we'd have this data. Then we'd have expectations in Blink unittests which were dependent on the ICU version, but those may already exist too...
Nevermind, we already have this in our checkout. :) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...
It looks like Chrome has crazy sophisticated architecture for looking up test-paths from the test server. DIR_TEST_DATA, etc. :)
Taught the harness how to skip tests with the new isolate directives (which Blink's UBA doesn't understand yet), and we still have quite a few failures: WARNING: Skipped 418143 tests. Ran 352098 tests: 44887 level failures, 19153 order failures.
PTAL. I think we can actually get this reviewed and landed now. These tests unfortunately won't run on the bots yet, but getting some version of this harness checked in is a huge step forward.
Neat. https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... File Source/platform/text/BidiResolverTest.cpp (right): https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:74: void runTest(const std::basic_string<UChar>& input, const std::vector<int>& reorder, std::basic_string<UChar> <--- Hot. We should have a typedef for that eventually. (Definitely not needed in this CL.) https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:86: // Blink's UBA does not filter out control characters, etc. Maybe it should? UBA <--- ? Maybe explain the acronym the first time? https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:217: // bots we just print a warning that we can't find the test file. We can expose something in webkit_support if we need to.
On 2013/10/28 00:26:46, abarth wrote: > Neat. > > https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... > File Source/platform/text/BidiResolverTest.cpp (right): > > https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... > Source/platform/text/BidiResolverTest.cpp:74: void runTest(const > std::basic_string<UChar>& input, const std::vector<int>& reorder, > std::basic_string<UChar> <--- Hot. We should have a typedef for that > eventually. (Definitely not needed in this CL.) I'm actually cheating a bit in my "destined for unicode.org" BidiTestHarness.h code by using UChar, since it's an ICU type. But I agree, we should consider using more std:: in Blink, and part of that is having a way to get a std:: string of UChars. :) > https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... > Source/platform/text/BidiResolverTest.cpp:86: // Blink's UBA does not filter out > control characters, etc. Maybe it should? > UBA <--- ? Maybe explain the acronym the first time? This whole file is about testing the UBA (Unicode Bidi Algorithm), but I explained it better in the comment. :) > https://codereview.chromium.org/39523002/diff/380001/Source/platform/text/Bid... > Source/platform/text/BidiResolverTest.cpp:217: // bots we just print a warning > that we can't find the test file. > We can expose something in webkit_support if we need to. Tell me more! Currently webkit_support.h isn't included by any part of blink... but if you can explain to me where I should add the API, I'm happy to move towards that. Possibly in a follow-up CL.
On 2013/10/28 01:08:43, eseidel wrote: > Tell me more! Currently webkit_support.h isn't included by any part of blink... > but if you can explain to me where I should add the API, I'm happy to move > towards that. Possibly in a follow-up CL. It looks like this stuff flows through WebUnitTestSupport.h these days: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... // Returns the root directory of the WebKit code. virtual WebString webKitRootDir() { return WebString(); } You could add something that returns the chromiumRootDir().
I tried that, but unfortunately that depends on being part of webkit_unit_tests, which this test is not. I could add the Platform setup bits to blink_platform_unittests, but that seems ugly.
On 2013/10/28 04:34:46, eseidel wrote: > I tried that, but unfortunately that depends on being part of webkit_unit_tests, > which this test is not. I could add the Platform setup bits to > blink_platform_unittests, but that seems ugly. We want to do that eventually, but we don't need to do that for this CL.
If igor or levi could review the BidiResolver hooks, abarth I'm sure is happy to review the unittest integration. I believe this is ready for review and landing. :)
I'm not quite sure who should review my crazy std:: c++... but anyone is welcome to. I suspect if unicode.org is willing to take ownership of that file it probably will need further cleaning.
Crap. I just found http://www.unicode.org/Public/PROGRAMS/BidiReferenceC/6.3.0/source/brtest.c I wonder if that would have worked instead of writing my own parser. :(
It doesn't look like that would be a drop-in replacement (it too is rather tied to its UBA implementation), but it might have been a better place to start.
Adding jyasskin to take a look at the C++ stl kung fu. I'm really excited about this landing. The integration with Blink's UBA LGTM. I'm relying on Adam and Jeff for the overall unit-test semantics and C++ stuff respectively. https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... File Source/platform/text/BidiResolverTest.cpp (right): https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:86: // Blink's UBA does not filter out control characters, etc. Maybe it should? Do you mean that we put them into the BidiRuns? If took them out, we'd potentially have to create additional runs (to have the offsets balance around them). It's not entirely clear to me what we'd get as a benefit. Either way, if we filtered them, it likely wouldn't be in TextRun(), which is how you're creating these. I think this is a fine solution for now. https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:96: // But it seems to expect LRI, etc. to be rendered!? Wat? This seems so weird... https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:101: std::ostringstream diff; Seems like you may just want "using namespace std" in this file :) https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:13: * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY Nit: We don't need to use the "Provided by Apple" license. https://codereview.chromium.org/39523002/diff/640002/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:91: s.erase(s.begin(), std::find_if(s.begin(), s.end(), std::not1(std::ptr_fun<int, int>(std::isspace)))); jyasskin for this and the other std C++ stuff.
Updated the license per levi's review. I also spoke with jyasskin briefly offline and he recommended I remove most of the stream usage, which I've done. (I'm happy to remove more if you like.)
https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:34: #include <fstream> I don't think you use this header: just #include <istream>? https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:92: inline std::string& ltrim(std::string& s) I'd either trim the argument in place or take the argument by const& and return the trimmed string by value. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:121: return true; If this function can't return false, I'd return the std::vector<std::string> instead of passing it as an out-parameter. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:137: static bool parseLevelRule(std::string line, std::vector<int>& levels) Generally pass strings as const std::string& to avoid copies. The only time you'd pass strings by value is when the function is going to make a copy as part of its internal work anyway. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:163: if (charClassExamples.empty()) { This isn't thread-safe, which you should comment. You can make it thread-safe for everything except MSVC<2013 if you write a function that returns the map, and initialize the local static with that. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:200: modeMask = atoi(line.c_str()); One cool thing you can do in C++ is use bitset<3> to store a set of 3 bits, instead of using bits in an int: http://en.cppreference.com/w/cpp/utility/bitset. You'd probably typedef it next to the enum in order to avoid duplicating the "3". enum ParagraphDirection { DirectionAutoLTR, DirectionLTR, DirectionRTL, }; typedef std::bitset<3> ParagraphDirectionSet; It has a constructor from a long long, so you'd use: int modeMask = atoi(line.c_str()); out_param = ParagraphDirectionSet(modeMask); return out_param.count() > 0 && modeMask < (1<<out_param.size()); You can use strtol() to get more error checking, since atoi() doesn't tell you if the argument it bigger than an int or has trailing characters. But you may not care. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:207: printf("Parse error, line %d : %s\n", lineNumber, line.c_str()); I think you're missing #include <stdio.h> https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:213: std::string levelsPrefix("@Levels"); Make these const if you're not planning to change them. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:228: line = originalLine; I'd usually declare variables where they're initialized, when possible. You can even use "std::getline(bidiTestFile, line) ... const std::string originalLine = line;" to emphasize that 'originalLine isn't going to change. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:253: if (!parseTestString(line.substr(0, seperatorIndex), testString)) { sp: seperator https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:262: if (paragraphDirectionMask & DirectionAutoLTR) With a bitset, this becomes paragraphDirectionMask.test(DirectionAutoLTR) or paragraphDirectionMask(DirectionAutoLTR). I tend to find that easier to read than using the same type for bit names and sets of bits, but if you don't, feel free to ignore the bitset comments. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:263: m_runner.runTest(testString, reorder, levels, DirectionAutoLTR, originalLine, lineNumber); It doesn't look like |levels| has a value coming into this function. Is the next m_runner.runTest invocation using the resulting value? If not, I'd probably lean toward making |levels| a local variable in runTest.
Thanks for the fantastic comments. Posting an update shortly. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:34: #include <fstream> On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > I don't think you use this header: just #include <istream>? Done. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:92: inline std::string& ltrim(std::string& s) On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > I'd either trim the argument in place or take the argument by const& and return > the trimmed string by value. Done. I did the in-place way as it was simpler. I'm sure we could write a nicer trim... https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:121: return true; On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > If this function can't return false, I'd return the std::vector<std::string> > instead of passing it as an out-parameter. Done. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:137: static bool parseLevelRule(std::string line, std::vector<int>& levels) On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > Generally pass strings as const std::string& to avoid copies. The only time > you'd pass strings by value is when the function is going to make a copy as part > of its internal work anyway. Done. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:163: if (charClassExamples.empty()) { On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > This isn't thread-safe, which you should comment. You can make it thread-safe > for everything except MSVC<2013 if you write a function that returns the map, > and initialize the local static with that. Done. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:200: modeMask = atoi(line.c_str()); On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > One cool thing you can do in C++ is use bitset<3> to store a set of 3 bits, > instead of using bits in an int: > http://en.cppreference.com/w/cpp/utility/bitset. You'd probably typedef it next > to the enum in order to avoid duplicating the "3". > > enum ParagraphDirection { > DirectionAutoLTR, > DirectionLTR, > DirectionRTL, > }; > typedef std::bitset<3> ParagraphDirectionSet; > > It has a constructor from a long long, so you'd use: > int modeMask = atoi(line.c_str()); > out_param = ParagraphDirectionSet(modeMask); > return out_param.count() > 0 && modeMask < (1<<out_param.size()); > > You can use strtol() to get more error checking, since atoi() doesn't tell you > if the argument it bigger than an int or has trailing characters. But you may > not care. Although these are both awesome suggestions, I'm going to defer them to possible later cleanup. There is still another parser to write with this code. :) https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:207: printf("Parse error, line %d : %s\n", lineNumber, line.c_str()); On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > I think you're missing #include <stdio.h> Done. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:213: std::string levelsPrefix("@Levels"); On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > Make these const if you're not planning to change them. Made them "static const". https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:228: line = originalLine; On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > I'd usually declare variables where they're initialized, when possible. You can > even use "std::getline(bidiTestFile, line) ... const std::string originalLine = > line;" to emphasize that 'originalLine isn't going to change. That makes an extra copy of line though, no? https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:253: if (!parseTestString(line.substr(0, seperatorIndex), testString)) { On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > sp: seperator I'm considering taking 3rd grade again as a correspondence class. :( https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:263: m_runner.runTest(testString, reorder, levels, DirectionAutoLTR, originalLine, lineNumber); On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > It doesn't look like |levels| has a value coming into this function. Is the next > m_runner.runTest invocation using the resulting value? If not, I'd probably lean > toward making |levels| a local variable in runTest. It's confusing. "levels" and "order" are both "global" variables with regards to the parser. They're set each time @Levels or @Reorder is encountered and then all successive lines use those values until they're changed.
Updated, PTAL.
lgtm https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:228: line = originalLine; On 2013/10/29 20:49:44, eseidel wrote: > On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > > I'd usually declare variables where they're initialized, when possible. You > can > > even use "std::getline(bidiTestFile, line) ... const std::string originalLine > = > > line;" to emphasize that 'originalLine isn't going to change. > > That makes an extra copy of line though, no? It shouldn't make any more copies than you have in your current code: std::string line; while (std::getline(bidiTestFile, line)) { // Copy from input. const std::string originalLine = line; // 1 copy of contents, here or at the first change to |line|. https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:263: m_runner.runTest(testString, reorder, levels, DirectionAutoLTR, originalLine, lineNumber); On 2013/10/29 20:49:44, eseidel wrote: > On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > > It doesn't look like |levels| has a value coming into this function. Is the > next > > m_runner.runTest invocation using the resulting value? If not, I'd probably > lean > > toward making |levels| a local variable in runTest. > > It's confusing. "levels" and "order" are both "global" variables with regards > to the parser. They're set each time @Levels or @Reorder is encountered and > then all successive lines use those values until they're changed. Oh, right, I just missed the assignment above because Rietveld wrapped the line and broke my ctrl-f. https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... File Source/platform/text/BidiResolverTest.cpp (right): https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiResolverTest.cpp:225: std::ifstream bidiTestFile(bidiTestPath); This needs the #include <fstream>; it's the other file that only uses istreams. https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:113: size_t lastPos = str.find_first_not_of(seperators); // skip leading spaces "seperators" still isn't spelled right. :) https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:154: // This static is not thread-safe, but currently that's not an issue. Oh, I meant you should comment the function as not thread-safe, since callers have to be aware of that. https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:229: levels = parseLevels(line.substr(levelsPrefix.length() + 1)); Note that line.substr makes a copy of the data. You can't do any better in just the standard library, since we don't have a string_ref class, but if performance turns out to matter, it may be worth writing one yourself.
On 2013/10/29 23:16:02, Jeffrey Yasskin wrote: > lgtm > > https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... > File Source/platform/text/BidiTestHarness.h (right): > > https://codereview.chromium.org/39523002/diff/770001/Source/platform/text/Bid... > Source/platform/text/BidiTestHarness.h:228: line = originalLine; > On 2013/10/29 20:49:44, eseidel wrote: > > On 2013/10/29 20:03:22, Jeffrey Yasskin wrote: > > > I'd usually declare variables where they're initialized, when possible. You > > can > > > even use "std::getline(bidiTestFile, line) ... const std::string > originalLine > > = > > > line;" to emphasize that 'originalLine isn't going to change. > > > > That makes an extra copy of line though, no? > > It shouldn't make any more copies than you have in your current code: > > std::string line; > while (std::getline(bidiTestFile, line)) { // Copy from input. > const std::string originalLine = line; // 1 copy of contents, here or at the > first change to |line|. I had no idea std::string was COW!?
https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... File Source/platform/text/BidiTestHarness.h (right): https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:113: size_t lastPos = str.find_first_not_of(seperators); // skip leading spaces On 2013/10/29 23:16:03, Jeffrey Yasskin wrote: > "seperators" still isn't spelled right. :) I checked a dictionary this time. separators :) https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:154: // This static is not thread-safe, but currently that's not an issue. On 2013/10/29 23:16:03, Jeffrey Yasskin wrote: > Oh, I meant you should comment the function as not thread-safe, since callers > have to be aware of that. Done. https://codereview.chromium.org/39523002/diff/810001/Source/platform/text/Bid... Source/platform/text/BidiTestHarness.h:229: levels = parseLevels(line.substr(levelsPrefix.length() + 1)); On 2013/10/29 23:16:03, Jeffrey Yasskin wrote: > Note that line.substr makes a copy of the data. You can't do any better in just > the standard library, since we don't have a string_ref class, but if performance > turns out to matter, it may be worth writing one yourself. Performance only sorta matters for this code. Ideally we'd make it fast if it were to be used by many projects, but just being used for one unittest it doesn't really matter that that unittest currently takes 20s to run on my machine. :(
On 2013/10/30 22:59:34, eseidel wrote:
> On 2013/10/29 23:16:02, Jeffrey Yasskin wrote:
> > std::string line;
> > while (std::getline(bidiTestFile, line)) { // Copy from input.
> > const std::string originalLine = line; // 1 copy of contents, here or at
> the
> > first change to |line|.
>
> I had no idea std::string was COW!?
Well, it's complicated. gcc's (libstdc++'s) std::string is COW, but C++11
actually bans that and requires eager copies, mostly because COW interacts in
some surprising ways with threads. Whenever libstdc++ decides to break ABI
compatibility, its std::string will stop being COW and will gain the
small-string optimization. libc++ and probably Microsoft's std::string are
already non-COW.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/860002
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1060001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/10/31 00:56:19, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on mac_layout. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. You need to switch to size_t for Mac's clang.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1210001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_blink_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1420001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1550001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1370002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_blink_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1670001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_blink_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/39523002/1740001
Message was sent while issue was closed.
Change committed as 161046 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
