Regkey functions return error code instead of bool

base/win/registry.h

Index: base/win/registry.h
===================================================================
--- base/win/registry.h	(revision 70414)
+++ base/win/registry.h	(working copy)
@@ -11,6 +11,35 @@
 
 #include "base/basictypes.h"
 
+// Please ignore this part. This temporary hack exists

[brettw, 2011/01/10 20:42:28]

What's the plan for removing this? Should it be a TODO?

[amit, 2011/01/12 04:11:23]

Todo added. 

This was to help catch all the usages and help catch new usages while merging. I
was planning to remove this before checkin. 

If you wish, I could leave this on for a day or two to protect others that have
changes in flight.

[Sigurður Ásgeirsson, 2011/01/12 14:04:19]

Maybe not a bad idea to leave this in place for a bit, but perhaps only in debug
mode?

+// to detect if the return value is used as 'bool'.
+struct CatchBoolChecks {
+  CatchBoolChecks(LONG l) : l_(l) {}
+  LONG l_;
+  operator LONG() { return l_; }
+  LONG value() const { return l_; }
+  bool operator == (LONG l) const { return l == l_; }
+  bool operator != (LONG l) const { return l != l_; }
+private:

[brettw, 2011/01/10 20:42:28]

Indent one more space.

[amit, 2011/01/12 04:11:23]

Done.

+  operator bool () {return false;}

[robertshield, 2011/01/10 21:34:37]

If you intend this to be checked in, consider adding a big visible comment
saying something like:
"If you hit a compile error here, you attempted to use the return value as a
bool. Don't do that."

[amit, 2011/01/12 04:11:23]

Done.

+};
+
+inline bool operator == (const LONG& l, const CatchBoolChecks& g) {
+  return g.value() == l;
+}
+
+inline bool operator != (const LONG& l, const CatchBoolChecks& g) {
+  return g.value() != l;
+}
+
+using std::ostream;
+inline ostream& operator <<(ostream &os, const CatchBoolChecks& g) {
+  os << g.value();
+  return os;
+}
+
+typedef CatchBoolChecks GONG;
+
 namespace base {
 namespace win {
 
@@ -23,51 +52,52 @@
   RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access);
   ~RegKey();
 
-  bool Create(HKEY rootkey, const wchar_t* subkey, REGSAM access);
+  GONG Create(HKEY rootkey, const wchar_t* subkey, REGSAM access);
 
-  bool CreateWithDisposition(HKEY rootkey, const wchar_t* subkey,
+  GONG CreateWithDisposition(HKEY rootkey, const wchar_t* subkey,
                              DWORD* disposition, REGSAM access);
 
-  bool Open(HKEY rootkey, const wchar_t* subkey, REGSAM access);
+  GONG Open(HKEY rootkey, const wchar_t* subkey, REGSAM access);
 
   // Creates a subkey or open it if it already exists.
-  bool CreateKey(const wchar_t* name, REGSAM access);
+  GONG CreateKey(const wchar_t* name, REGSAM access);
 
   // Opens a subkey
-  bool OpenKey(const wchar_t* name, REGSAM access);
+  GONG OpenKey(const wchar_t* name, REGSAM access);
 
   void Close();
 
   DWORD ValueCount() const;
 
   // Determine the nth value's name.
-  bool ReadName(int index, std::wstring* name) const;
+  GONG ReadName(int index, std::wstring* name) const;
 
   // True while the key is valid.
   bool Valid() const { return key_ != NULL; }
 
   // Kill a key and everything that live below it; please be careful when using
   // it.
-  bool DeleteKey(const wchar_t* name);
+  GONG DeleteKey(const wchar_t* name);
 
   // Deletes a single value within the key.
-  bool DeleteValue(const wchar_t* name);
+  GONG DeleteValue(const wchar_t* name);
 
-  bool ValueExists(const wchar_t* name);
+  bool ValueExists(const wchar_t* name) const;
 
-  bool ReadValue(const wchar_t* name, void* data, DWORD* dsize,
+  GONG ReadValue(const wchar_t* name, void* data, DWORD* dsize,
                  DWORD* dtype) const;
-  bool ReadValue(const wchar_t* name, std::wstring* value) const;
-  bool ReadValueDW(const wchar_t* name, DWORD* value) const;
+  GONG ReadValue(const wchar_t* name, std::wstring* value) const;
+  GONG ReadValueDW(const wchar_t* name, DWORD* value) const;

[Sigurður Ásgeirsson, 2011/01/10 21:02:43]

The way you use this in ceee relies upon the fact that *value is untouched
except on success. Since this isn't documented or tested, it falls into
implementation detail.
Either change the usage or (better) document and test this behavior? 

[amit, 2011/01/12 04:11:23]

I think it's not proper for an API to return failure but cause side effects in
the return arg. So if it's OK with you, I would prefer to document, test and
enforce this behavior.

[Sigurður Ásgeirsson, 2011/01/12 14:04:19]

I couldn't agree more - SGTM!

+  GONG ReadValueQW(const wchar_t* name, int64* value) const;

[brettw, 2011/01/10 20:42:28]

What do you think about calling this ReadValue64 or even ReadInt64? We've never
used quadword in our codebase so this looks a bit out of place, whereas "64" is
much more common.

[amit, 2011/01/12 04:11:23]

Yeah ReadInt64 is much more readable. Changed.

 
-  bool WriteValue(const wchar_t* name, const void* data, DWORD dsize,
+  GONG WriteValue(const wchar_t* name, const void* data, DWORD dsize,
                   DWORD dtype);
-  bool WriteValue(const wchar_t* name, const wchar_t* value);
-  bool WriteValue(const wchar_t* name, DWORD value);
+  GONG WriteValue(const wchar_t* name, const wchar_t* value);
+  GONG WriteValue(const wchar_t* name, DWORD value);
 
   // Starts watching the key to see if any of its values have changed.
   // The key must have been opened with the KEY_NOTIFY access privilege.
-  bool StartWatching();
+  GONG StartWatching();
 
   // If StartWatching hasn't been called, always returns false.
   // Otherwise, returns true if anything under the key has changed.
@@ -76,7 +106,7 @@
 
   // Will automatically be called by destructor if not manually called
   // beforehand.  Returns true if it was watching, false otherwise.
-  bool StopWatching();
+  GONG StopWatching();
 
   inline bool IsWatching() const { return watch_event_ != 0; }
   HANDLE watch_event() const { return watch_event_; }