Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8344004:
base/win: Add documentation to RegKey::Read/Write functions. (Closed)
|
Created:
9 years, 2 months ago by tfarina Modified:
9 years, 2 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, Erik does not do reviews, brettw-cc_chromium.org, dmaclach+watch_chromium.org, mihaip+watch_chromium.org, garykac+watch_chromium.org, Aaron Boodman, lambroslambrou+watch_chromium.org, robertshield, Paweł Hajdan Jr., ajwong+watch_chromium.org, sergeyu+watch_chromium.org, amit Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionbase/win: Add documentation to RegKey::Read/Write functions.
R=maruel@chromium.org,willchan@chromium.org,grt@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106869
Patch Set 1 : #Patch Set 2 : out/in_value #Patch Set 3 : fix indentation #
Total comments: 5
Patch Set 4 : just docs #
Total comments: 6
Patch Set 5 : add corner case comment #Messages
Total messages: 19 (0 generated)
PTAL! The only "interesting" change is in registry.h, registry.cc is just reordering the methods to match with the new order in the header file, and the other changes are just renames and fixing indentation. Thanks!
http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h#newcode72 base/win/registry.h:72: LONG ReadValue(const wchar_t* name, DWORD* out_value) const; Don't. Function overloading is banned by style. I don't think this change is valuable at all.
http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h#newcode72 base/win/registry.h:72: LONG ReadValue(const wchar_t* name, DWORD* out_value) const; On 2011/10/18 18:01:58, Marc-Antoine Ruel wrote: > Don't. > > Function overloading is banned by style. > Yeah I know :( > I don't think this change is valuable at all. Hum I did with the intention to make it consistent with WriteValue and to clean this mess.
http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h#newcode72 base/win/registry.h:72: LONG ReadValue(const wchar_t* name, DWORD* out_value) const; On 2011/10/18 18:03:27, tfarina wrote: > I did with the intention to make it consistent with WriteValue and to clean this > mess. Then fix WriteValue() to not use function overloading, not the reverse.
http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h#newcode72 base/win/registry.h:72: LONG ReadValue(const wchar_t* name, DWORD* out_value) const; On 2011/10/18 18:05:17, Marc-Antoine Ruel wrote: > On 2011/10/18 18:03:27, tfarina wrote: > > I did with the intention to make it consistent with WriteValue and to clean > this > > mess. > > Then fix WriteValue() to not use function overloading, not the reverse. OK, I'll hold off in renaming them for now (it breaks rlz library anyway :( ). I'll just add the docs for now.
Marc, just the docs changes now. Please, could you take another look?
http://codereview.chromium.org/8344004/diff/3028/base/win/registry.cc File base/win/registry.cc (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.cc#newcode145 base/win/registry.cc:145: LONG RegKey::ReadValueDW(const wchar_t* name, DWORD* out_value) const { What makes this ordering better than the previous one? It's not sorted by name or anything. I prefer not to move code 'just to move it' since it breaks svn blame/git blame. http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h#newcode69 base/win/registry.h:69: // Getters: I feel like the comments don't add much. The signature is describing what it does well enough. That's probably there wasn't in the first place. Sincerely, I'm not sure about this change at all.
http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/5029/base/win/registry.h#newcode72 base/win/registry.h:72: LONG ReadValue(const wchar_t* name, DWORD* out_value) const; On 2011/10/18 18:01:58, Marc-Antoine Ruel wrote: > Don't. > > Function overloading is banned by style. > Hum, reading it again, it doesn't explicit bans function overload. It says: "You may write a function that takes a const string& and overload it with another that takes const char*." http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h#newcode69 base/win/registry.h:69: // Getters: On 2011/10/18 18:34:14, Marc-Antoine Ruel wrote: > I feel like the comments don't add much. The signature is describing what it > does well enough. That's probably there wasn't in the first place. Sincerely, > I'm not sure about this change at all. Like I said in the first email I was trying to make it closer to Omaha API. And it seems is just me that is seeing the benefit to make this clean (easier to read) and not a *mess* like it's now! And yes the order is particular, it is in an order of complexity (int, int64, string, ...); Please, take a look at their API here: http://code.google.com/p/omaha/source/browse/trunk/base/reg_key.h
On 2011/10/18 18:40:43, tfarina wrote: > Please, take a look at their API here: > http://code.google.com/p/omaha/source/browse/trunk/base/reg_key.h Both are a fork of an internal file. Their code doesn't respect the style guide as the original one didn't.
On 2011/10/18 18:45:25, Marc-Antoine Ruel wrote: > On 2011/10/18 18:40:43, tfarina wrote: > > Please, take a look at their API here: > > http://code.google.com/p/omaha/source/browse/trunk/base/reg_key.h > > Both are a fork of an internal file. Their code doesn't respect the style guide > as the original one didn't. OK, but I think that doesn't mean the documentation doesn't have its value. Will and grt may agree or not with that too. I hope if you look more careful you will see that(I was not expecting this reaction to this patch, though). I'd like to hear their inputs too.
http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h#newcode71 base/win/registry.h:71: // Returns an int32 value. Comments here would be useful to explain the corner cases: e.g., I assume that null or empty string for |name| means read the key's default value. The comment doesn't say if one or both is valid, so I have to look in the implementation to figure it out.
http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h#newcode71 base/win/registry.h:71: // Returns an int32 value. On 2011/10/18 19:59:33, grt wrote: > Comments here would be useful to explain the corner cases: e.g., I assume that > null or empty string for |name| means read the key's default value. Exactly. If |name| is NULL or empty it retrieves the default value, if any. And it will returns ERROR_FILE_NOT_FOUND if the value for |name| does not exist. > The comment > doesn't say if one or both is valid, so I have to look in the implementation to > figure it out.
http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h File base/win/registry.h (right): http://codereview.chromium.org/8344004/diff/3028/base/win/registry.h#newcode71 base/win/registry.h:71: // Returns an int32 value. On 2011/10/18 19:59:33, grt wrote: > Comments here would be useful to explain the corner cases: e.g., I assume that > null or empty string for |name| means read the key's default value. The comment > doesn't say if one or both is valid, so I have to look in the implementation to > figure it out. Comment added. Please, take another look.
On 2011/10/19 01:39:07, tfarina wrote: > Comment added. Please, take another look. Those comments look fine. I think maruel's concern about breaking git/svn blame is valid. I don't feel terribly strongly about it, but if you're going to take the effort to clean up the comments, it'd be nice to do so for the whole file. Given that this class is a really thin wrapper around the Win32 API and generally requires knowledge of that API, I don't know that the effort involved will really save many programmer-hours in the future. In short, I respect your will to clean it up, but I don't know that it's worth it. If you feel strongly about it, you have my LGTM.
On 2011/10/20 20:11:00, grt wrote: > On 2011/10/19 01:39:07, tfarina wrote: > > Comment added. Please, take another look. > > Those comments look fine. I think maruel's concern about breaking git/svn blame > is valid. I don't feel terribly strongly about it, but if you're going to take > the effort to clean up the comments, it'd be nice to do so for the whole file. > Given that this class is a really thin wrapper around the Win32 API and > generally requires knowledge of that API, I don't know that the effort involved > will really save many programmer-hours in the future. In short, I respect your > will to clean it up, but I don't know that it's worth it. If you feel strongly > about it, you have my LGTM. I can revert the move changes in the source file if that hurts much. I don't care too much either. Is just my obsession to keep everything in order and symmetric. Will, can I get your OWNERS approve?
On 2011/10/21 20:20:52, tfarina wrote: > On 2011/10/20 20:11:00, grt wrote: > > On 2011/10/19 01:39:07, tfarina wrote: > > > Comment added. Please, take another look. > > > > Those comments look fine. I think maruel's concern about breaking git/svn > blame > > is valid. I don't feel terribly strongly about it, but if you're going to > take > > the effort to clean up the comments, it'd be nice to do so for the whole file. > > > Given that this class is a really thin wrapper around the Win32 API and > > generally requires knowledge of that API, I don't know that the effort > involved > > will really save many programmer-hours in the future. In short, I respect > your > > will to clean it up, but I don't know that it's worth it. If you feel > strongly > > about it, you have my LGTM. > > I can revert the move changes in the source file if that hurts much. I don't > care too much either. Is just my obsession to keep everything in order and > symmetric. > > Will, can I get your OWNERS approve? resending to see if Rietveld fires an email this time...
lgtm |
