Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694693002:
Roll re2 dba3349..596d73e. (Closed)
DescriptionRoll re2 dba3349..596d73e.
https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba3349..596d73e
R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org
BUG=690374, 691352
Review-Url: https://codereview.chromium.org/2694693002
Cr-Commit-Position: refs/heads/master@{#450362}
Committed: https://chromium.googlesource.com/chromium/src/+/980055357a756285706684b55181827f8614e709
Patch Set 1 #
Total comments: 4
Patch Set 2 : More fixes from the upstream #Patch Set 3 : Another rebase #Patch Set 4 : Use unsigned literal when asserting re2::StringPiece::size() value. #
Messages
Total messages: 37 (22 generated)
Description was changed from ========== Roll re2 dba3349..aa627d9. R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374 ========== to ========== Roll re2 dba3349..aa627d9. https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba... R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374 ==========
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... File testing/libfuzzer/fuzzers/re2_fuzzer.cc (left): https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... testing/libfuzzer/fuzzers/re2_fuzzer.cc:16: #include "util/logging.h" Is this change and the other below necessary for the roll? https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... File third_party/re2/README.chromium (left): https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... third_party/re2/README.chromium:14: To update RE2, execute the following commands from your Chromium checkout: Why are you removing these instructions?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... File testing/libfuzzer/fuzzers/re2_fuzzer.cc (left): https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... testing/libfuzzer/fuzzers/re2_fuzzer.cc:16: #include "util/logging.h" On 2017/02/13 10:09:05, tfarina wrote: > Is this change and the other below necessary for the roll? Yes. This header doesn't exist anymore. Due to that, line #73 is not valid anymore as well. It is being used to suppress logging, but now it is being suppressed by default. https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... File third_party/re2/README.chromium (left): https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... third_party/re2/README.chromium:14: To update RE2, execute the following commands from your Chromium checkout: On 2017/02/13 10:09:05, tfarina wrote: > Why are you removing these instructions? These instructions are not relevant anymore. It used to be relevant before https://codereview.chromium.org/1544433002, when we had to update all the files manually. Now it can be done much easier via DEPS.
On 2017/02/13 10:15:47, mmoroz wrote: > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > File testing/libfuzzer/fuzzers/re2_fuzzer.cc (left): > > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > testing/libfuzzer/fuzzers/re2_fuzzer.cc:16: #include "util/logging.h" > On 2017/02/13 10:09:05, tfarina wrote: > > Is this change and the other below necessary for the roll? > > Yes. This header doesn't exist anymore. Due to that, line #73 is not valid > anymore as well. It is being used to suppress logging, but now it is being > suppressed by default. > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > File third_party/re2/README.chromium (left): > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > third_party/re2/README.chromium:14: To update RE2, execute the following > commands from your Chromium checkout: > On 2017/02/13 10:09:05, tfarina wrote: > > Why are you removing these instructions? > > These instructions are not relevant anymore. It used to be relevant before > https://codereview.chromium.org/1544433002, when we had to update all the files > manually. Now it can be done much easier via DEPS. I've filed a bug for compilation errors: https://github.com/google/re2/issues/132 Hopefully it should be fixed quickly and I'll change the CL to roll the next revision or so.
Description was changed from ========== Roll re2 dba3349..aa627d9. https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba... R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374 ========== to ========== Roll re2 dba3349..596d73e. https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba... R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374,691352 ==========
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
On 2017/02/13 10:42:34, mmoroz wrote: > On 2017/02/13 10:15:47, mmoroz wrote: > > > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > > File testing/libfuzzer/fuzzers/re2_fuzzer.cc (left): > > > > > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > > testing/libfuzzer/fuzzers/re2_fuzzer.cc:16: #include "util/logging.h" > > On 2017/02/13 10:09:05, tfarina wrote: > > > Is this change and the other below necessary for the roll? > > > > Yes. This header doesn't exist anymore. Due to that, line #73 is not valid > > anymore as well. It is being used to suppress logging, but now it is being > > suppressed by default. > > > > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > > File third_party/re2/README.chromium (left): > > > > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > > third_party/re2/README.chromium:14: To update RE2, execute the following > > commands from your Chromium checkout: > > On 2017/02/13 10:09:05, tfarina wrote: > > > Why are you removing these instructions? > > > > These instructions are not relevant anymore. It used to be relevant before > > https://codereview.chromium.org/1544433002, when we had to update all the > files > > manually. Now it can be done much easier via DEPS. > > I've filed a bug for compilation errors: > https://github.com/google/re2/issues/132 > > Hopefully it should be fixed quickly and I'll change the CL to roll the next > revision or so. Comparison of different types in //base/logging.h... Sigh..
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
On 2017/02/14 10:00:00, mmoroz wrote: > On 2017/02/13 10:42:34, mmoroz wrote: > > On 2017/02/13 10:15:47, mmoroz wrote: > > > > > > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > > > File testing/libfuzzer/fuzzers/re2_fuzzer.cc (left): > > > > > > > > > https://codereview.chromium.org/2694693002/diff/1/testing/libfuzzer/fuzzers/r... > > > testing/libfuzzer/fuzzers/re2_fuzzer.cc:16: #include "util/logging.h" > > > On 2017/02/13 10:09:05, tfarina wrote: > > > > Is this change and the other below necessary for the roll? > > > > > > Yes. This header doesn't exist anymore. Due to that, line #73 is not valid > > > anymore as well. It is being used to suppress logging, but now it is being > > > suppressed by default. > > > > > > > > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > > > File third_party/re2/README.chromium (left): > > > > > > > > > https://codereview.chromium.org/2694693002/diff/1/third_party/re2/README.chro... > > > third_party/re2/README.chromium:14: To update RE2, execute the following > > > commands from your Chromium checkout: > > > On 2017/02/13 10:09:05, tfarina wrote: > > > > Why are you removing these instructions? > > > > > > These instructions are not relevant anymore. It used to be relevant before > > > https://codereview.chromium.org/1544433002, when we had to update all the > > files > > > manually. Now it can be done much easier via DEPS. > > > > I've filed a bug for compilation errors: > > https://github.com/google/re2/issues/132 > > > > Hopefully it should be fixed quickly and I'll change the CL to roll the next > > revision or so. > > Comparison of different types in //base/logging.h... Sigh.. Ok I got it. re2::StringPiece::size() now returns different type.
Current version with `size_t` (https://chromium.googlesource.com/external/github.com/google/re2.git/+blame/m...) looks better than an old one with `int`.
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mmoroz@chromium.org changed reviewers: + engedy@chromium.org
On 2017/02/14 13:44:29, mmoroz wrote: > Current version with `size_t` > (https://chromium.googlesource.com/external/github.com/google/re2.git/+blame/m...) > looks better than an old one with `int`. Adding engedy@ for one-line change in password_manager :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 13:50:31, mmoroz wrote: > On 2017/02/14 13:44:29, mmoroz wrote: > > Current version with `size_t` > > > (https://chromium.googlesource.com/external/github.com/google/re2.git/+blame/m...) > > looks better than an old one with `int`. > > Adding engedy@ for one-line change in password_manager :) Woohoo, Dry Run is passed! Please take a look :)
Woohoo, shiny new RE2! Thanks and LGTM on password_manager.
lgtm
lgtm
The CQ bit was checked by mmoroz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487086534200970,
"parent_rev": "57ddf4790da08494139fa5615835785c23753bef", "commit_rev":
"980055357a756285706684b55181827f8614e709"}
Message was sent while issue was closed.
Description was changed from ========== Roll re2 dba3349..596d73e. https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba... R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374,691352 ========== to ========== Roll re2 dba3349..596d73e. https://chromium.googlesource.com/external/github.com/google/re2.git/+log/dba... R=inferno@chromium.org, tfarina@chromium.org, thakis@chromium.org BUG=690374,691352 Review-Url: https://codereview.chromium.org/2694693002 Cr-Commit-Position: refs/heads/master@{#450362} Committed: https://chromium.googlesource.com/chromium/src/+/980055357a756285706684b55181... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/980055357a756285706684b55181...
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
