Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2974783002:
Fixed some nits based on reviewer comments. (Closed)
DescriptionFixed some nits based on reviewer comments here:
https://codereview.chromium.org/2977433002/
and here:
https://codereview.chromium.org/2946063002/
BUG=
Review-Url: https://codereview.chromium.org/2974783002
Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d
Review-Url: https://codereview.chromium.org/2974783002
Committed: https://github.com/google/syzygy/commit/1242df937a63aaafa8a1ba9a2c7d18885bf6e18a
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed some nits based on reviewer comments. #
Total comments: 1
Patch Set 3 : Fixed some nits based on reviewer comments. #Messages
Total messages: 28 (17 generated)
Description was changed from ========== Fixed some nits based on reviewer comments. BUG= ========== to ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= ==========
njanevsk@google.com changed reviewers: + etienneb@chromium.org, sebmarchand@chromium.org
PTAL
lgtm https://codereview.chromium.org/2974783002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2974783002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:123: os.path.splitext(os.path.basename(source_file))[0])) This should be aligned with the 'target_name' argument.
The CQ bit was checked by njanevsk@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by njanevsk@google.com
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": 1, "attempt_start_ts": 1499709892885270, "parent_rev":
"28b6e488e884e65a32865d38f02c9a3e39d82100", "commit_rev":
"ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d"}
Message was sent while issue was closed.
Description was changed from ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= ========== to ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2981443002/ by sebmarchand@chromium.org. The reason for reverting is: Please don't commit until you've addressed all the comments..
Message was sent while issue was closed.
PTAL https://codereview.chromium.org/2974783002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2974783002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:123: os.path.splitext(os.path.basename(source_file))[0])) On 2017/07/10 16:46:10, Sébastien Marchand wrote: > This should be aligned with the 'target_name' argument. Done.
Message was sent while issue was closed.
Please re-open the CL , go to "Edit Issue" and uncheck the "Closed" box. https://codereview.chromium.org/2974783002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2974783002/diff/20001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:123: os.path.basename(source_file))[0])) The problem is the same here, this is aligned to the parenthesis of another function. I'll align this and the previous 'os.path.dirname' to +4 (relative to the 'return' above)
Message was sent while issue was closed.
On 2017/07/10 18:46:52, Sébastien Marchand wrote: > Please re-open the CL , go to "Edit Issue" and uncheck the "Closed" box. > > https://codereview.chromium.org/2974783002/diff/20001/syzygy/integration_test... > File syzygy/integration_tests/make_integration_tests_clang.py (right): > > https://codereview.chromium.org/2974783002/diff/20001/syzygy/integration_test... > syzygy/integration_tests/make_integration_tests_clang.py:123: > os.path.basename(source_file))[0])) > The problem is the same here, this is aligned to the parenthesis of another > function. I'll align this and the previous 'os.path.dirname' to +4 (relative to > the 'return' above) Done. Btw, is it acceptable in cases like this where there is a long line of code to declare variables before and then use them making the line shorter and easier to read. I know some people and coding styles are against use of variables if they are gonna be used only once. I personally prefer sometimes to use extra variables to make lines shorter.
Message was sent while issue was closed.
Yeah, it's fine to use extra variables if it improves readibility, in this case it's good like this I think. You didn't reopened the CL, you'll need to do this to commit it. lgtm
Message was sent while issue was closed.
Description was changed from ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d ========== to ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d ==========
The CQ bit was checked by njanevsk@google.com
The CQ bit was unchecked by njanevsk@google.com
The CQ bit was checked by njanevsk@google.com
The CQ bit was unchecked by njanevsk@google.com
The CQ bit was checked by njanevsk@google.com
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": 40001, "attempt_start_ts": 1499713825229920,
"parent_rev": "645f2dfea673cb5051a9cc73a2870036e3afe1e0", "commit_rev":
"1242df937a63aaafa8a1ba9a2c7d18885bf6e18a"}
Message was sent while issue was closed.
Description was changed from ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d ========== to ========== Fixed some nits based on reviewer comments here: https://codereview.chromium.org/2977433002/ and here: https://codereview.chromium.org/2946063002/ BUG= Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/ca5c6ffffaa76f7348be08dab6ba21bcadcc5a0d Review-Url: https://codereview.chromium.org/2974783002 Committed: https://github.com/google/syzygy/commit/1242df937a63aaafa8a1ba9a2c7d18885bf6e18a ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/google/syzygy/commit/1242df937a63aaafa8a1ba9a2c7d18885bf6e18a |
