Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1352613004:
Cleanup: Pass std::string as const reference from ash/ + minor coding style changes in base/ (Closed)
DescriptionCleanup: Pass std::string as const reference from ash/ + minor coding style changes in base/
Passing std::string by reference can prevent extra copying of object.
BUG=367418
TEST=
R=derat@chromium.org,sky@chromium.org,mark@chromium.org,stevenjb@chromium.org
Committed: https://crrev.com/b6cbc047deeaf616ec2181f270fff6eaf0eb3443
Cr-Commit-Position: refs/heads/master@{#350593}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Allocate |device_id| on the stack #
Total comments: 2
Patch Set 3 : Remove const qualifier for int in function arguments #
Messages
Total messages: 31 (10 generated)
The change in base is a no-op and doesn’t have anything to do with the change description.
On 2015/09/21 00:22:30, Mark Mentovai - August is over wrote: > The change in base is a no-op and doesn’t have anything to do with the change > description. I added a comment about changes in base/
please add the most-specific owners that you can. in the case of ash/system/OWNERS, this would be someone from this list: sadrul@chromium.org stevenjb@chromium.org jennyz@chromium.org skuhne@chromium.org https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... File ash/system/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... ash/system/bluetooth/tray_bluetooth.cc:378: const std::string& device_id = find->second; this makes me nervous. if one of the methods called below deletes this entry from device_map_, then the reference will become invalid and there's the potential for a use-after-free. i'd prefer that you kept the copy here.
ki.stfu@gmail.com changed reviewers: + stevenjb@chromium.org
On 2015/09/21 23:06:46, ki.stfu wrote: > mailto:ki.stfu@gmail.com changed reviewers: > + mailto:stevenjb@chromium.org stevenjb@: added as reviewer for ash/system/bluetooth/tray_bluetooth.cc
https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... File ash/system/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... ash/system/bluetooth/tray_bluetooth.cc:378: const std::string& device_id = find->second; On 2015/09/21 14:12:45, Daniel Erat wrote: > this makes me nervous. if one of the methods called below deletes this entry > from device_map_, then the reference will become invalid and there's the > potential for a use-after-free. i'd prefer that you kept the copy here. +1. For this kind of minor optimization cleanup I would really prefer to error on the side of being overly cautious.
https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... File ash/system/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/1352613004/diff/1/ash/system/bluetooth/tray_b... ash/system/bluetooth/tray_bluetooth.cc:378: const std::string& device_id = find->second; On 2015/09/21 14:12:45, Daniel Erat wrote: > this makes me nervous. if one of the methods called below deletes this entry > from device_map_, then the reference will become invalid and there's the > potential for a use-after-free. i'd prefer that you kept the copy here. Done.
lgtm
LGTM in base, on the basis of this matching the prevailing style in this file and in Chromium.
lgtm too https://codereview.chromium.org/1352613004/diff/20001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/1352613004/diff/20001/base/logging.h#newcode809 base/logging.h:809: inline void LogAtLevel(const int log_level, const std::string& msg) { (i don't know why the int parameter was const in the first place)
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352613004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352613004/20001
https://codereview.chromium.org/1352613004/diff/20001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/1352613004/diff/20001/base/logging.h#newcode809 base/logging.h:809: inline void LogAtLevel(const int log_level, const std::string& msg) { On 2015/09/23 19:20:39, Daniel Erat wrote: > (i don't know why the int parameter was const in the first place) +1
The CQ bit was unchecked by ki.stfu@gmail.com
The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352613004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352613004/20001
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, stevenjb@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/1352613004/#ps40001 (title: "Remove const qualifier for int in function arguments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352613004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352613004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352613004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352613004/40001
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 ki.stfu@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352613004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352613004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b6cbc047deeaf616ec2181f270fff6eaf0eb3443 Cr-Commit-Position: refs/heads/master@{#350593} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
