Add a ToString function to create human-readable debug descriptions #2384

var-const · 2019-02-13T01:20:42Z

The motivation is to avoid writing lots of boilerplate for containers.

var-const · 2019-02-13T01:24:18Z

Firestore/core/src/firebase/firestore/util/type_traits.h

-  using value_type = bool;
-
-  enum { value = sizeof(Choose(Pointer())) == sizeof(yes_type) };
+struct is_objective_c_pointer : std::is_convertible<T, id> {};


I think this is equivalent to the previous implementation (and the tests pass), but I'll run it by Gil before merging.

If TEST(TypeTraitsTest, IsObjectiveCPointer) passes I'm all for having less code.

Could this be a using statement? Something like using is_objective_c_pointer = std::is_convertible<T, id>?

Done, good idea.

var-const · 2019-02-13T17:57:07Z

Also removed the conversion from DocumentKey to FSTDocumentKey. I noticed that ToString takes performing the conversion and calling description: on that over calling ToString member function. This could be changed in the implementation, but I found the conversion surprising and unexpected, so I preferred to simply eliminate it.

rsgowman · 2019-02-13T19:39:56Z

Also removed the conversion from DocumentKey to FSTDocumentKey.

Change seems reasonable, but mostly unrelated to this PR. I'd prefer if it was separate.

I noticed that ToString takes performing the conversion and calling description: on that over calling ToString member function. This could be changed in the implementation,

See my comment. tl;dr: I kinda think ToString member function should take priority regardless of this particular issue. (But it's your call.)

but I found the conversion surprising and unexpected, so I preferred to simply eliminate it.

Well, we, explicitly added that at some point for ease of porting. The last commit in this PR otherwise adds a bunch of boilerplate back in (though makes things more obvious). I'm neutral as to where we end up on that trade-off, but if we do that with this type, we should probably do that will all of them. Either way, that discussion's probably better served via a separate PR. (... unless this is the only type where we do that?)

rsgowman · 2019-02-13T17:16:20Z

Firestore/core/src/firebase/firestore/util/to_string.h

+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TO_STRING_H_
+
+#if !defined(__OBJC__)
+#error "This header only supports Objective-C++"


You could split this into to_string.h and to_string_apple.h (or do some ifdef magic around the objc specificities). There's no real urgent need to do so, but it'd make this useful from the pure c++ world too.

Optional. If you do decide to do this, doing this as a separate PR would be reasonable.

Don't bother splitting. Just add #if defined(__OBJC__) around the relevant parts.

Using a mechanism like the one using FormatChoice in string_format.h makes this easier to do.

Done with ifdefs.

Firestore/core/src/firebase/firestore/util/to_string.h

rsgowman · 2019-02-13T18:44:52Z

Firestore/core/src/firebase/firestore/util/to_string.h

+       {1, "foo"},
+       {2, "bar"}
+ * };
+ * assert(ToString(m) == "{1: foo, 2: bar}");


An unordered map might be interesting... (Or at least difficult to test. :)

Done (sort of).

Ha. That's a pretty good way to dodge the issue. :)

rsgowman · 2019-02-13T19:01:24Z

Firestore/core/test/firebase/firestore/util/to_string_apple_test.mm

+  using MapT = SortedMap<int, std::string>;
+  MapT sorted_map = MapT{}.insert(1, "foo").insert(2, "bar");
+  EXPECT_EQ(ToString(sorted_map), "{1: foo, 2: bar}");
+}


Consider SortedSet too.

rsgowman · 2019-02-13T19:06:12Z

Firestore/core/test/firebase/firestore/util/to_string_apple_test.mm

+      {2, DocumentKey({"foo", "baz"})},
+  };
+  EXPECT_EQ(ToString(key_map), "{1: foo/bar, 2: foo/baz}");
+}


Consider some of the other standard containers. (You could test vector, but it's pretty well covered by the others. list seems somewhat uninteresting. But maybe set?)

As mentioned above, the unordered containers would be challenging to test. You could do some regex magic, but it's probably not worth it.

Added. I cheated by using unoredred_multimap.

rsgowman · 2019-02-13T19:27:12Z

Firestore/core/src/firebase/firestore/util/to_string.h

+ *    `std::string`;
+ *
+ *  - otherwise, if `value` defines a member function called `ToString`, the
+ *    description is created by invoking the function;


If a type is both a (custom) container and defines ToString, then this priority list implies that ToString won't be called. It would seem like ToString() should be given the highest priority rather than the lowest?

Regardless of where you end up on the above question, if you care about this ordering, consider adding a test for it.

Done. The ordering is now:

ToString

description

conversion to std::string;

assoc. container;

container;

std::to_string.

What do you think?

Test -- added a simple test. I mostly care about the ordering between associative and simple containers, because it's the only one that will always "conflict", but it's implicitly tested already.

New ordering seems reasonable. And yeah, the ordering between assoc/simple containers is definitely already tested. The one I was referring to, was the ToString + container ordering, which you've now added. ltgm.

wilhuff

but I found the conversion surprising and unexpected, so I preferred to simply eliminate it.

Well, we, explicitly added that at some point for ease of porting. The last commit in this PR otherwise adds a bunch of boilerplate back in (though makes things more obvious). I'm neutral as to where we end up on that trade-off, but if we do that with this type, we should probably do that will all of them. Either way, that discussion's probably better served via a separate PR. (... unless this is the only type where we do that?)

We added this initially because FSTDocumentKey was so prevalent and the implicit conversions were a crutch that made incremental progress possible. Implicit conversions are otherwise undesirable so if we're at a state where we can reasonably remove them I'm all for it.

wilhuff · 2019-02-13T19:34:50Z

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -717,6 +719,7 @@
 		54740A561FC913EB00713A1A /* util */ = {
 			isa = PBXGroup;
 			children = (
+				B68B1E002213A764008977EF /* to_string_apple_test.mm */,


If you run scripts/sync_project.rb does this change?

wilhuff · 2019-02-13T19:37:37Z

Firestore/core/src/firebase/firestore/immutable/sorted_map.h

@@ -17,6 +17,7 @@
 #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_H_
 #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_H_

+#include <type_traits>


Did you mean to include util/type_traits.h?

Leftover, sorry. I tried adding some static_asserts, but that didn't turn out to be very useful, because compilation attempts proceed after hitting a static_assert, drowning it in a pool of useless noise.

Firestore/core/src/firebase/firestore/util/to_string.h

wilhuff · 2019-02-13T19:50:15Z

Firestore/core/src/firebase/firestore/util/to_string.h

+// Fallback
+
+template <typename T>
+std::string ToStringDefault(const T& value) {


You're mixing "ToString" as a prefix and as a suffix in these member names. Shouldn't these be DefaultToString and CustomToString to match the others below?

var-const

@rsgowman I mainly did the DocumentKey change in this PR because FSTDocumentKey converts to a string differently, and I didn't want to modify the tests. Let me know if you feel strongly about this, but it's easier to make both changes in a single PR.

var-const · 2019-02-13T19:57:17Z

Firestore/core/test/firebase/firestore/util/to_string_apple_test.mm

+  using MapT = SortedMap<int, std::string>;
+  MapT sorted_map = MapT{}.insert(1, "foo").insert(2, "bar");
+  EXPECT_EQ(ToString(sorted_map), "{1: foo, 2: bar}");
+}


var-const · 2019-02-13T19:57:41Z

Firestore/core/test/firebase/firestore/util/to_string_apple_test.mm

+      {2, DocumentKey({"foo", "baz"})},
+  };
+  EXPECT_EQ(ToString(key_map), "{1: foo/bar, 2: foo/baz}");
+}


Added. I cheated by using unoredred_multimap.

var-const · 2019-02-13T20:07:52Z

Firestore/core/src/firebase/firestore/util/to_string.h

+ *    `std::string`;
+ *
+ *  - otherwise, if `value` defines a member function called `ToString`, the
+ *    description is created by invoking the function;


Done. The ordering is now:

ToString

description

conversion to std::string;

assoc. container;

container;

std::to_string.

What do you think?

Test -- added a simple test. I mostly care about the ordering between associative and simple containers, because it's the only one that will always "conflict", but it's implicitly tested already.

var-const · 2019-02-13T20:19:31Z

Firestore/core/src/firebase/firestore/util/to_string.h

+       {1, "foo"},
+       {2, "bar"}
+ * };
+ * assert(ToString(m) == "{1: foo, 2: bar}");


Done (sort of).

Firestore/core/src/firebase/firestore/util/to_string.h

var-const · 2019-02-13T21:08:04Z

Firestore/core/src/firebase/firestore/immutable/sorted_map.h

@@ -17,6 +17,7 @@
 #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_H_
 #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_IMMUTABLE_SORTED_MAP_H_

+#include <type_traits>


Leftover, sorry. I tried adding some static_asserts, but that didn't turn out to be very useful, because compilation attempts proceed after hitting a static_assert, drowning it in a pool of useless noise.

Firestore/core/src/firebase/firestore/util/to_string.h

var-const · 2019-02-13T21:12:53Z

Firestore/core/src/firebase/firestore/util/to_string.h

+// Fallback
+
+template <typename T>
+std::string ToStringDefault(const T& value) {


var-const · 2019-02-13T21:13:39Z

Firestore/core/src/firebase/firestore/util/type_traits.h

-  using value_type = bool;
-
-  enum { value = sizeof(Choose(Pointer())) == sizeof(yes_type) };
+struct is_objective_c_pointer : std::is_convertible<T, id> {};


Done, good idea.

rsgowman · 2019-02-13T21:29:53Z

I mainly did the DocumentKey change in this PR because FSTDocumentKey converts to a string differently, and I didn't want to modify the tests. Let me know if you feel strongly about this, but it's easier to make both changes in a single PR.

That's seems reasonable. (I was reading it as almost entirely unrelated.)

Firestore/core/src/firebase/firestore/util/to_string.h

wilhuff · 2019-02-13T22:31:14Z

Firestore/core/test/firebase/firestore/util/to_string_test.cc

+using model::DocumentKey;
+
+TEST(ToStringTest, StdToString) {
+  EXPECT_EQ(ToString(123), "123");


std::to_string isn't guaranteed to be specialized for all types, so I would expect this to fail for booleans and any trivial implementation would accidentally capture non-Objective-C pointer types. Please add tests.

StringFormat handles this case specially in order to give reasonable formats for things that are exactly bool but not convertible to bool: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/core/src/firebase/firestore/util/string_format.h#L72

Ideally we'd have similar behavior here.

This brings me to what is perhaps an unfair criticism of this change: it's unfortunate that this implementation is completely separate from StringFormat. Is there any way to reuse FormatArg here so that we don't need to re-implement all these transformations here?

Good point. I'm delegating to FormatArg instead of std::to_string now, what do you think? Also, added a test -- let me know if you can think of any additional cases worth testing.

wilhuff · 2019-02-14T01:37:07Z

Firestore/core/src/firebase/firestore/util/to_string.h

+ *  - if `value` defines a member function called `ToString`, the description is
+ *    created by invoking the function;
+ *
+ *  - (in Objective-C++ only) otherwise, if `value` is an Objective-C class, the


This comment isn't quite right. I think you mean "if value is an Objective-C object" rather than "class" since it's the type of the instance that's interesting rather than just classes themselves.

Done. I think it was supposed to be "if value is of an Objective-C class", but it's probably awkward, so I used your suggestion.

wilhuff · 2019-02-14T01:42:11Z

Firestore/core/src/firebase/firestore/util/to_string.h

+// Objective-C class
+
+template <typename T>
+std::string ObjCToString(const T& value, std::false_type) {


It's a little hard to follow the precedence in here because CustomToString changes its definition based on __OBJC__. Do you think it would be clearer to always define ObjcToString but if not __OBJC__ always delegate down?

Something like this (untested):

template <typename T, typename Tag> std::string ObjCToString(const T& value, Tag tag) { return StringToString([value description], tag); }

That may not work though because tag dispatch may get defeated in the presence of an actual value.

However, as I'm wrestling with this I end up coming back to the idea that the FormatChoice mechanism used in StringFormat ends up being clearer:

The choices read in the natural order

The conditions that affect whether or not a choice is viable are right above it (rather than in the caller)

Alternatives that don't apply on a platform can be harmlessly #ifed out without affecting readability

Only one method signature is required per level

There's no std::true_type and std::false_type tags to interpret.

The downside is std::enable_if but I think the price to pay isn't that bad, and we already have the type metamagic figured out--it's just a matter of moving it from the caller to the type declared above the alternative. Compare:

#if __OBJC__ template <typename T> std::string ObjCToString(const T& value, std::false_type) { return StringToString(value, std::is_same<T, std::string>{}); } template <typename T> std::string ObjCToString(const T& value, std::true_type) { return MakeString([value description]); } template <typename T> std::string CustomToString(const T& value, std::false_type) { return ObjCToString(value, is_objective_c_pointer<T>{}); } #else template <typename T> std::string CustomToString(const T& value, std::false_type) { return StringToString(value, std::is_same<T, std::string>{}); } #endif // __OBJC__

vs

#if __OBJC__ template < typename T, typename = typename std::enable_if<is_objective_c_pointer<T>{}>::type> std::string ToString(T object, internal::ToStringChoice<1>) { return MakeString([value description]); } #endif

Do you think it would be clearer to always define ObjcToString but if not __OBJC__ always delegate down?

Another way would be something like

template <typename T> std::string ObjCToString(const T& value, std::false_type) { return StringToString(value, std::is_same<T, std::string>{}); } #if __OBJC__ template <typename T> std::string ObjCToString(const T& value, std::true_type) { return MakeString([value description]); } #endif template <typename T> std::string CustomToString(const T& value, std::false_type) { // This would require a different is_objective_c_pointer definition // in pure C++, or another ifdef block here return ObjCToString(value, is_objective_c_pointer<T>::value); }

I don't have a strong preference on this mechanism versus FormatChoice. I agree with all your points except the last one (to me, there's no significant difference between FormatChoice<0> and std::true_type). It's certainly good for readability that the order isn't reversed and there's less boilerplate. On the other hand, the way FormatChoice works is more complicated. There's recursive inheritance, but more significantly, every function there is a potential candidate for the overload set, so it's necessary to remember to apply enable_if appropriately (but not always necessary, e.g., the const char* overload doesn't have it). Also, adding a new element to the chain would require modifying several indices as well as their total number (though it's not a huge deal since the number is small).

I tried out the FormatChoice approach, what do you think? Since I don't have a strong preference, I'll go with whichever you prefer.

zxu123

LGTM

wilhuff

LGTM.

wilhuff · 2019-02-15T00:10:16Z

Firestore/core/src/firebase/firestore/util/to_string.h

+
+}  // namespace impl
+
+/**


Documentation comments are usually better off on the declaration than the definition when they're separate.

wilhuff · 2019-02-15T00:11:37Z

Firestore/core/src/firebase/firestore/util/to_string.h

+  std::string contents = absl::StrJoin(
+      value, ", ", [](std::string* out, const typename T::value_type& kv) {
+        out->append(
+            StringFormat("%s: %s", ToString(kv.first), ToString(kv.second)));


StringFormat is pretty heavyweight for this, and it would be useful to make these implementations not mutually recursive. Consider just using three append calls.

* master: E2e tests split (#2401) Split auth api tests into multiple cases (#2399) Add a ToString function to create human-readable debug descriptions (#2384) Split Auth unit tests (#2396) Create umbrella header for FIAM (#2392) Deprecate setMinimumSessionInterval (#2305) Forbid queries endAt an uncommitted server timestamp. (#2382) update changelog for release (#2383) Fix FIAM Travic CI issues (#2380) Remove swift sample for Auth (#2371) C++ migration: port `FSTTransaction` (#2362) Fix a bug where unlinking emailpassword doesn’t remove provider data (#2370) Fix a bug where sign in with Game Center doesn’t return additional user info (#2368) Firebase In-app messaging callbacks (#2354) Fix a bug where sign in with email link always return isNewUser as false (#2363) C++ migration: eliminate `FSTRemoteStore` (#2338)

var-const added 2 commits February 12, 2019 20:16

Initial

3215889

Fix

07758ea

googlebot added the cla: yes label Feb 13, 2019

var-const requested review from zxu123 and rsgowman February 13, 2019 01:22

var-const assigned rsgowman and zxu123 Feb 13, 2019

var-const added the api: firestore label Feb 13, 2019

var-const commented Feb 13, 2019

View reviewed changes

var-const added 2 commits February 12, 2019 20:24

style.sh

0a1b145

Remove DocumentKey->FSTDocumentKey conversion

3428867

rsgowman reviewed Feb 13, 2019

View reviewed changes

rsgowman assigned var-const and unassigned rsgowman Feb 13, 2019

wilhuff reviewed Feb 13, 2019

View reviewed changes

var-const added 3 commits February 13, 2019 15:19

Review feedback

ce8da11

Review feedback

4084c57

Review feedback

f39d23f

var-const commented Feb 13, 2019

View reviewed changes

var-const assigned rsgowman and wilhuff and unassigned var-const Feb 13, 2019

rsgowman approved these changes Feb 13, 2019

View reviewed changes

rsgowman removed their assignment Feb 13, 2019

wilhuff reviewed Feb 13, 2019

View reviewed changes

var-const added 2 commits February 13, 2019 19:53

Review feedback

193301f

style.sh

026f517

var-const added 2 commits February 13, 2019 19:56

Review feedback

fadadb9

Quick fix

6b157c4

wilhuff approved these changes Feb 14, 2019

View reviewed changes

zxu123 approved these changes Feb 14, 2019

View reviewed changes

zxu123 assigned var-const and unassigned zxu123 Feb 14, 2019

wilhuff removed their assignment Feb 14, 2019

var-const added 2 commits February 14, 2019 17:05

Review feedback

ea32eab

Review feedback

baff0e6

var-const assigned wilhuff and unassigned var-const Feb 14, 2019

wilhuff approved these changes Feb 15, 2019

View reviewed changes

var-const added 2 commits February 15, 2019 13:53

Fix

292cdcd

Review feedback

eaa8f5f

var-const assigned var-const and unassigned wilhuff Feb 15, 2019

var-const merged commit 5efbb08 into master Feb 15, 2019

paulb777 deleted the varconst/to-string branch October 2, 2019 19:20

firebase locked and limited conversation to collaborators Oct 22, 2019

Add a ToString function to create human-readable debug descriptions #2384

Add a ToString function to create human-readable debug descriptions #2384

Conversation

var-const commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const commented Feb 13, 2019

rsgowman commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsgowman commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxu123 left a comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const commented Feb 13, 2019 •

edited

Loading