Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2977433002:
Fixed few small bugs (Closed)
|
Created:
3 years, 5 months ago by njanevsk Modified:
3 years, 5 months ago Reviewers:
Sébastien Marchand CC:
syzygy-changes_googlegroups.com, chrisha, etienneb Target Ref:
refs/heads/master Project:
syzygy Visibility:
Public. |
DescriptionFixed few small bugs
BUG=
Review-Url: https://codereview.chromium.org/2977433002
Committed: https://github.com/google/syzygy/commit/28b6e488e884e65a32865d38f02c9a3e39d82100
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed some nits based on reviewer comments. #Messages
Total messages: 14 (5 generated)
Description was changed from ========== Fixed few small bugs BUG= ========== to ========== Fixed few small bugs BUG= ==========
njanevsk@google.com changed reviewers: + sebmarchand@chromium.org
This should work without any modifications to the gyp file. PTAL.
lgtm https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:61: print compile_command Remove the debugging code.
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": 1499454812568850, "parent_rev":
"d98b9eeb23efbbfa81fcca245c7ef68d0ec5028c", "commit_rev":
"28b6e488e884e65a32865d38f02c9a3e39d82100"}
Message was sent while issue was closed.
Description was changed from ========== Fixed few small bugs BUG= ========== to ========== Fixed few small bugs BUG= Review-Url: https://codereview.chromium.org/2977433002 Committed: https://github.com/google/syzygy/commit/28b6e488e884e65a32865d38f02c9a3e39d82100 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://github.com/google/syzygy/commit/28b6e488e884e65a32865d38f02c9a3e39d82100
Message was sent while issue was closed.
https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:61: print compile_command On 2017/07/07 19:11:22, Sébastien Marchand wrote: > Remove the debugging code. You forgot to address this comment. Please make sure to address all comments before committing your code.
Message was sent while issue was closed.
https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:59: compile_command = compile_command_base This doesn't work as this doesn't copy the list, see https://stackoverflow.com/questions/2612802/how-to-clone-or-copy-a-list (use the list() function). This was obvious when executing the script due to the logging below, please make sure to test the code and ensure that it's doing what it should do before submitting.
Message was sent while issue was closed.
On 2017/07/10 15:10:04, Sébastien Marchand wrote: > https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... > File syzygy/integration_tests/make_integration_tests_clang.py (right): > > https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... > syzygy/integration_tests/make_integration_tests_clang.py:59: compile_command = > compile_command_base > This doesn't work as this doesn't copy the list, see > https://stackoverflow.com/questions/2612802/how-to-clone-or-copy-a-list (use the > list() function). > > This was obvious when executing the script due to the logging below, please make > sure to test the code and ensure that it's doing what it should do before > submitting. Also address the comments from Etienne here: https://codereview.chromium.org/2946063002/
Message was sent while issue was closed.
https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:59: compile_command = compile_command_base On 2017/07/10 15:10:04, Sébastien Marchand wrote: > This doesn't work as this doesn't copy the list, see > https://stackoverflow.com/questions/2612802/how-to-clone-or-copy-a-list (use the > list() function). > > This was obvious when executing the script due to the logging below, please make > sure to test the code and ensure that it's doing what it should do before > submitting. Actually the code does what it is supposed to do. I tested it and checked that it generates all the object files and the dll :-D However, you are right that the code is better/more correct if the list() is used. https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:61: print compile_command On 2017/07/07 19:11:22, Sébastien Marchand wrote: > Remove the debugging code. Done. https://codereview.chromium.org/2977433002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:61: print compile_command On 2017/07/10 14:58:42, Sébastien Marchand wrote: > On 2017/07/07 19:11:22, Sébastien Marchand wrote: > > Remove the debugging code. > > You forgot to address this comment. Please make sure to address all comments > before committing your code. Done.
Message was sent while issue was closed.
This CL has already been committed, please fix the comments in another one. |
