dbus: add ObjectPath type

dbus/object_path.h

Index: dbus/object_path.h
diff --git a/dbus/object_path.h b/dbus/object_path.h
new file mode 100644
index 0000000000000000000000000000000000000000..3c055a6ac636fa714cd313a5b851a47a42103904
--- /dev/null
+++ b/dbus/object_path.h
@@ -0,0 +1,58 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DBUS_OBJECT_PATH_H_
+#define DBUS_OBJECT_PATH_H_
+#pragma once
+
+#include <ostream>
+#include <string>
+
+namespace dbus {
+
+// ObjectPath is a type used to distinguish D-Bus object paths from simple
+// strings, especially since normal practice is that these should be only
+// initialized from static constants or obtained from remote objects and no
+// assumptions about their value made.
+//
+// An ObjectPath can be initialized from a std::string or from a character
+// array, may be copied and assigned new vlaues, and may be used as a key
+// in a std::map.
+//
+// To obtain the object path value as a std::string use value(), to obtain
+// as a C-string (e.g. to pass to libdbus functions) use c_str().
+class ObjectPath {
+ public:
+  // Permit initialization, both implicit and explicit, without a value
+  // (for passing to dbus::MessageReader::PopObjectPath), from std::string
+  // objects, and from string constants.
+  //
+  // The compiler synthesised copy constructor and assignment operator are
+  // sufficient for our needs.
+  ObjectPath() {}
+  ObjectPath(const std::string& value) : value_(value) {}

[satorux1, 2012/02/12 20:16:30]

explicit is missing.

[keybuk, 2012/02/12 22:09:29]

see the comment - allowing implicit construction is explicitly intended here so
that you can do:

  bus->GetObjectProxy(kServiceManagerPath...)

which requires implicit construction of an object path from a constant.

The coding style allows this -- "Classes that are intended to be transparent
wrappers around other classes are also exceptions" -- and I wanted this to be as
transparent and painless to use as possible :)

[satorux1, 2012/02/13 19:07:36]

On the contrary, I don't want to make it transparent for consistency with other
pieces of code in Chrome. Except for few places like implementation of
base::Bind(), Chrome code prefers making things clearer or very obvious over
making code succinct. In other words, less magic is preferred, which is also the
philosophy of the C++ style guide.

For instance, FilePath class in 'base' is conceptually similar to ObjectPath
here, but does not allow the implicit conversion from string to FilePath (i.e.
they use 'explicit' in the constructor.

That said, clients code will look like:

  bus->GetObjectProxy(ObjectPath(kServiceManagerPath), ...)

which is less succinct, but clearer about what's happening. 

+  ObjectPath(const char* c_str) : value_(c_str) {}

[satorux1, 2012/02/12 20:16:30]

I think we don't need the const char* version. The std::string version should
allow clients to pass string literals.

[keybuk, 2012/02/12 22:09:29]

yeah, I was convinced of that, but it doesn't work in the case above...

dbus::ObjectPath manager(kServiceManagerPath) will work because it has a const
char *, and expects a std::string, and can call a std::string into existence to
fulfill the constructor

but dbus::ObjectPath manager = kServiceManagerPath will not work, because the
compiler will not call intermediate type into existence

+
+  // Retrieve value as a std::string and as a C-style string.

[satorux1, 2012/02/12 20:16:30]

Retrieve -> Retrieves

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

+  const std::string& value() const { return value_; }
+  const char* c_str() const { return value_.c_str(); }

[satorux1, 2012/02/12 20:16:30]

I think we can drop c_str(). Clients can just go with value().c_str(). Less API
would be desirable.

[keybuk, 2012/02/12 22:09:29]

sure thing, only useful inside dbus:: anyway (which is the only place that turns
them into C strings)

+ private:
+  std::string value_;
+};
+
+// Permit sufficient comparison to allow an ObjectPath to be used as a
+// key in a std::map.
+bool operator<(const ObjectPath&, const ObjectPath&);
+
+// Permit testing for equality, required for mocks to work and useful for
+// observers.
+bool operator==(const ObjectPath&, const ObjectPath&);
+bool operator!=(const ObjectPath&, const ObjectPath&);

[satorux1, 2012/02/12 20:16:30]

Can we go with Equals() instead?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

[keybuk, 2012/02/12 22:09:29]

note the "required for mocks to work" in the comment ;) since mocks don't
compile without these, I figured commenting that they were also useful for
observers didn't hurt

[satorux1, 2012/02/13 19:07:36]

BTW, FilePath has these operators inside the class. Can we follow the style?

I vaguely remember that the "effective C++" discussed on this topic, but I don't
have a copy right now.

[keybuk, 2012/02/13 20:04:42]

yup, I missed out the "const" from them when I tried it, which is why it didn't
work - will fix

+
+// Permit direct outputting on streams, for logging and debugging.
+std::ostream& operator<<(std::ostream&, const ObjectPath&);

[satorux1, 2012/02/12 20:16:30]

Let's drop this. Per our C++ style guide, we rarely define this. Since we
already have value() that returns a string, we can just go with LOG(INFO) <<
object_path.value().

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

However, there may be rare cases where you need to overload an operator to
interoperate with templates or "standard" C++ classes (such as
operator<<(ostream&, const T&) for logging). These are acceptable if fully
justified, but you should try to avoid these whenever possible.

[keybuk, 2012/02/12 22:09:29]

my hope is that we can actually get rid of value() entirely, code in general
shouldn't be peeking at the values of object paths and should treat them as
opaque types retrieved from dbus calls or instantiated from constants

there's a few places that won't work right now (mostly inside dbus::), but I'm
happy to file a bug to fix those so we can drop value() entirely, in which case
the logging operator is useful

[satorux1, 2012/02/13 19:07:36]

For the same reason mentioned above, making it transparent is not desirable.

[keybuk, 2012/02/13 20:04:42]

meh meh meh :p

+
+}  // namespace dbus
+
+#endif  // DBUS_OBJECT_PATH_H_