Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14265020:
Remove scoped_array usage from RegistryKeyBackup. (Closed)
|
Created:
7 years, 8 months ago by dcheng Modified:
7 years, 8 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove scoped_array usage from RegistryKeyBackup.
BUG=171111
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194672
Patch Set 1 #Patch Set 2 : Namespace adjustment #
Total comments: 38
Patch Set 3 : FIxups #Patch Set 4 : One more #
Total comments: 2
Patch Set 5 : Fix nits #Patch Set 6 : static_cast<DWORD> #Messages
Total messages: 6 (0 generated)
IMO, this is slightly clunky but I don't have much better ideas about this either.
This lg to me but would like Greg to take a look at this as well. Agree that it's a bit clunky in places. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... File chrome/installer/util/registry_key_backup.cc (right): https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:57: // Explicitly copyable for STL containers. nit: Isn't it rather implicit since it's not called out anywhere? https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:86: // Explicitly copyable for STL containers. nit: implicit, since the copy ctor etc are compiler generated.
looks good. there are two more uses of scoped_array that are easily removed; see below. please make sure installer_util_unittests still pass. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... File chrome/installer/util/registry_key_backup.cc (right): https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:57: // Explicitly copyable for STL containers. how about: // CopyConstructible and Assignable for use in STL containers. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:86: // Explicitly copyable for STL containers. how about: // CopyConstructible and Assignable for use in STL containers. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:110: { nit: move to previous line https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:137: scoped_array<wchar_t> name_buffer(new wchar_t[max_name_len]); std::vector<wchar_t> name_buffer(max_name_len); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:140: if (num_values != 0) { inside this block: values.reserve(num_values); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:141: scoped_array<uint8> value_buffer(new uint8[max_value_len]); std::vector<uint8> value_buffer(max_value_len ? max_value_len : 1); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:149: result = RegEnumValue(key.Handle(), i, name_buffer.get(), &name_size, name_buffer.get() -> &name_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:150: NULL, &value_type, value_buffer.get(), &value_size); value_buffer.get() -> &value_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:157: values.back().Initialize(name_buffer.get(), name_size, value_type, name_buffer.get() -> &name_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:158: value_buffer.get(), value_size); value_buffer.get() -> &value_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:164: value_buffer.reset(); // Release to heap before new allocation. replace this and the next line with: value_buffer.resize(max_value_len); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:173: name_buffer.reset(); // Release to heap before new allocation. replace this and the next line with: name_buffer.resize(max_name_len); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:183: DLOG_IF(WARNING, RegEnumValue(key.Handle(), num_values, name_buffer.get(), name_buffer.get() -> &name_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:196: result = RegEnumKeyEx(key.Handle(), i, name_buffer.get(), &name_size, name_buffer.get() -> &name_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:203: subkeys[std::wstring(name_buffer.get(), name_size)] = KeyData(); name_buffer.get() -> &name_buffer[0] https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:203: subkeys[std::wstring(name_buffer.get(), name_size)] = KeyData(); nit: " = KeyData()" constructs an extra object that isn't needed. operator[] implicitly creates the entry with a default-constructed KeyData. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:212: name_buffer.reset(new wchar_t[max_name_len]); name_buffer.resize(max_name_len); https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:230: kKeyReadNoNotify); nit: move to previous line
PTAL. Note that I've removed code related to numeric_limits since a reading of http://msdn.microsoft.com/en-us/library/windows/desktop/ms724872(v=vs.85).aspx suggests that none of the values can ever exceed std::numeric_limits<DWORD>::max() anyway. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... File chrome/installer/util/registry_key_backup.cc (right): https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:57: // Explicitly copyable for STL containers. On 2013/04/16 14:28:25, grt wrote: > how about: > // CopyConstructible and Assignable for use in STL containers. Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:86: // Explicitly copyable for STL containers. On 2013/04/16 14:28:25, grt wrote: > how about: > // CopyConstructible and Assignable for use in STL containers. Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:110: { On 2013/04/16 14:28:25, grt wrote: > nit: move to previous line Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:137: scoped_array<wchar_t> name_buffer(new wchar_t[max_name_len]); On 2013/04/16 14:28:25, grt wrote: > std::vector<wchar_t> name_buffer(max_name_len); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:140: if (num_values != 0) { On 2013/04/16 14:28:25, grt wrote: > inside this block: > values.reserve(num_values); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:141: scoped_array<uint8> value_buffer(new uint8[max_value_len]); On 2013/04/16 14:28:25, grt wrote: > std::vector<uint8> value_buffer(max_value_len ? max_value_len : 1); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:149: result = RegEnumValue(key.Handle(), i, name_buffer.get(), &name_size, On 2013/04/16 14:28:25, grt wrote: > name_buffer.get() -> &name_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:150: NULL, &value_type, value_buffer.get(), &value_size); On 2013/04/16 14:28:25, grt wrote: > value_buffer.get() -> &value_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:157: values.back().Initialize(name_buffer.get(), name_size, value_type, On 2013/04/16 14:28:25, grt wrote: > name_buffer.get() -> &name_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:158: value_buffer.get(), value_size); On 2013/04/16 14:28:25, grt wrote: > value_buffer.get() -> &value_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:164: value_buffer.reset(); // Release to heap before new allocation. On 2013/04/16 14:28:25, grt wrote: > replace this and the next line with: > value_buffer.resize(max_value_len); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:173: name_buffer.reset(); // Release to heap before new allocation. On 2013/04/16 14:28:25, grt wrote: > replace this and the next line with: > name_buffer.resize(max_name_len); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:183: DLOG_IF(WARNING, RegEnumValue(key.Handle(), num_values, name_buffer.get(), On 2013/04/16 14:28:25, grt wrote: > name_buffer.get() -> &name_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:196: result = RegEnumKeyEx(key.Handle(), i, name_buffer.get(), &name_size, On 2013/04/16 14:28:25, grt wrote: > name_buffer.get() -> &name_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:203: subkeys[std::wstring(name_buffer.get(), name_size)] = KeyData(); On 2013/04/16 14:28:25, grt wrote: > nit: " = KeyData()" constructs an extra object that isn't needed. operator[] > implicitly creates the entry with a default-constructed KeyData. That's true, but it's kind of non-obvious. I've changed this into an explicit call to insert(). https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:203: subkeys[std::wstring(name_buffer.get(), name_size)] = KeyData(); On 2013/04/16 14:28:25, grt wrote: > name_buffer.get() -> &name_buffer[0] Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:212: name_buffer.reset(new wchar_t[max_name_len]); On 2013/04/16 14:28:25, grt wrote: > name_buffer.resize(max_name_len); Done. https://codereview.chromium.org/14265020/diff/2001/chrome/installer/util/regi... chrome/installer/util/registry_key_backup.cc:230: kKeyReadNoNotify); On 2013/04/16 14:28:25, grt wrote: > nit: move to previous line Done.
lgtm w/ two nits. https://codereview.chromium.org/14265020/diff/10001/chrome/installer/util/reg... File chrome/installer/util/registry_key_backup.cc (right): https://codereview.chromium.org/14265020/diff/10001/chrome/installer/util/reg... chrome/installer/util/registry_key_backup.cc:156: if (value_size > value_buffer.size()) { nit: no braces https://codereview.chromium.org/14265020/diff/10001/chrome/installer/util/reg... chrome/installer/util/registry_key_backup.cc:160: if (name_size > name_buffer.size() - 1) { nit: no braces
Message was sent while issue was closed.
Committed patchset #6 manually as r194672 (presubmit successful). |
