Port `Precondition` to C++ #1040

zxu123 · 2018-04-06T19:17:43Z

This is based on the branch cpp/port_field_transform. Merge to it for easier review; eventually will merge it to master once the base branch is merged.

Discussion

This is part of the C++ migration.

Testing

unit test and integration test pass.

API Changes

n/a

…ransform) to C++

zxu123 · 2018-04-06T19:22:41Z

There is one design change from FSTPrecondition to Precondition: FSTPrecondition has optional properties, where only one property set meaningful value, which determines the type of FSTPrecondition. Precondition instead explicitly has a type member to define its type. This is to solve the issue of no nil equivalent in C++ and IMHO is a more clean design.

rsgowman · 2018-04-06T19:22:38Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    UpdateTime,
+  };
+
+  /** Creates a new Precondition with an exists flag. */


nit: "Creates a new" is the wrong phrase when returning a reference. How about "Returns a Precondition with an exists flag." ?

OTOH, it's a little awkward to return a reference for this one, but to create Preconditions for UpdateTime(). As a result, it's unclear who owns these.

It looks like this class just has a few small private members. I'd be tempted to turn it into a value object, and not optimize via static instances. You'll have more Precondition objects floating around, but each one looks to be not much bigger than a reference anyways.

My recommendation: Leave the phrasing as is, but change Exists() and None() to just return Precondition rather than const Precondition&.

(Note that a lot of the calling code takes copies anyways, so if you dont want to make this change, you should double check the calling code to ensure the copies are ok.)

rsgowman · 2018-04-06T19:37:13Z

Firestore/core/src/firebase/firestore/model/precondition.h

+  static const Precondition& None();
+
+  /**
+   * Returns true if the preconditions is valid for the given document (or null


"or null if no document is available"

^ Needs updating.

Looking at the code, if no document is available, this returns false. How about:

"Returns true if the precondition is valid for the given document (and the document is available)"

(Also note the grammar nit: s/preconditions/precondition/)

rsgowman · 2018-04-06T19:52:49Z

Firestore/core/src/firebase/firestore/model/precondition.h

+        FIREBASE_ASSERT_MESSAGE(false, "Invalid precondition");
+        break;
+    }
+  }


nit: since your cases all return, I don't think you need 'break' anywhere. (Presumably you're including it for safety?)

nit: you've exhaustively handled all (reasonable) enum values. Rather than including a default, you could omit it, and then assert outside the switch (which, since all your cases return, should be unreachable.)

i.e.

switch (x) { case one_of_two: return 1; case two_of_two: return 2; } FIREBASE_UNREACHABLE();

This should cause the compiler to complain if new enum values are added without adjusting the switch. (Otherwise the 'default' case will mask that.)

rsgowman · 2018-04-06T19:53:54Z

Firestore/core/src/firebase/firestore/model/precondition.h

+  }
+
+  bool operator==(const Precondition& other) const {
+    return update_time_ == other.update_time_ && exists_ == other.exists_;


Should you also compare type_? (If yes, also adjust hash())

I see that:

the old code did not

it doesn't actually matter, due to the way the ctors are defined, there shouldn't be a case where two instances differ only via type_.

But it seems like it might be slightly safer in case the code changes in such a way as to otherwise allow this.

nit: Since you've updated operator==, you should also update Hash().

rsgowman · 2018-04-06T19:59:07Z

Firestore/core/src/firebase/firestore/model/precondition.h

+                             update_time_.timestamp().ToString().c_str()];
+        break;
+      default:
+        // We do not raise assertion here. This function is mainly used in


Seems reasonable.

Consider a dev assertion though. If this branch is ever executed, it might save us some grief.

rsgowman · 2018-04-06T20:01:02Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+    default:
+      FIREBASE_ASSERT_MESSAGE(false, "Invalid precondition");
+      break;
+  }


(See comment in header file.)

rsgowman · 2018-04-06T20:04:19Z

Firestore/core/src/firebase/firestore/model/snapshot_version.h

+#if defined(__OBJC__)
+  SnapshotVersion(FSTSnapshotVersion* version) {  // NOLINT(runtime/explicit)
+    if ([version isEqual:[FSTSnapshotVersion noVersion]]) {
+      timestamp_ = Timestamp{};


I think this is unnecessary, as the timestamp_ instance will already be initialized with it's default ctor. (No objections if you want to keep it though.)

rsgowman · 2018-04-06T20:08:02Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

@@ -40,6 +46,24 @@ inline model::ResourcePath Resource(absl::string_view field) {
  return model::ResourcePath::FromString(field);
 }

+// Version is epoch time in macroseconds. This helper is defined so to match


nit: s/macro/micro/

googlebot · 2018-04-09T13:52:44Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

zxu123

Updated. PTAL.

zxu123 · 2018-04-09T13:27:33Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    UpdateTime,
+  };
+
+  /** Creates a new Precondition with an exists flag. */


zxu123 · 2018-04-09T13:28:53Z

Firestore/core/src/firebase/firestore/model/precondition.h

+  static const Precondition& None();
+
+  /**
+   * Returns true if the preconditions is valid for the given document (or null


zxu123 · 2018-04-09T13:30:55Z

Firestore/core/src/firebase/firestore/model/precondition.h

+        FIREBASE_ASSERT_MESSAGE(false, "Invalid precondition");
+        break;
+    }
+  }


zxu123 · 2018-04-09T13:32:25Z

Firestore/core/src/firebase/firestore/model/precondition.h

+  }
+
+  bool operator==(const Precondition& other) const {
+    return update_time_ == other.update_time_ && exists_ == other.exists_;


zxu123 · 2018-04-09T13:35:05Z

Firestore/core/src/firebase/firestore/model/precondition.h

+                             update_time_.timestamp().ToString().c_str()];
+        break;
+      default:
+        // We do not raise assertion here. This function is mainly used in


zxu123 · 2018-04-09T13:37:55Z

Firestore/core/src/firebase/firestore/model/snapshot_version.h

+#if defined(__OBJC__)
+  SnapshotVersion(FSTSnapshotVersion* version) {  // NOLINT(runtime/explicit)
+    if ([version isEqual:[FSTSnapshotVersion noVersion]]) {
+      timestamp_ = Timestamp{};


zxu123 · 2018-04-09T13:38:30Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

@@ -40,6 +46,24 @@ inline model::ResourcePath Resource(absl::string_view field) {
  return model::ResourcePath::FromString(field);
 }

+// Version is epoch time in macroseconds. This helper is defined so to match


zxu123 · 2018-04-09T13:39:53Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+    default:
+      FIREBASE_ASSERT_MESSAGE(false, "Invalid precondition");
+      break;
+  }


rsgowman

One nit (re adding type_ to Hash()). Otherwise lgtm.

var-const

Some nits.

var-const · 2018-04-09T15:48:59Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    FIREBASE_UNREACHABLE();
+  }
+
+  bool operator==(const Precondition& other) const {


This seems like it could be useful outside #if defined(__OBJC__).

var-const · 2018-04-09T15:49:46Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    switch (type_) {
+      case Type::None:
+        return @"<Precondition <none>>";
+        break;


Nit: no need for break after a return statement.

Done. Really adding break do no harm but would catch bugs if later non-return code is added into the case. But I don't have strong opinion and thus removed.

I see your reasoning, thanks for explaining. I'm still leaning towards not having non-reachable code (and linters might complain).

var-const · 2018-04-09T15:51:14Z

Firestore/core/src/firebase/firestore/model/precondition.h

+                             update_time_.timestamp().ToString().c_str()];
+        break;
+      default:
+        // We only raise dev assertion here. This function is mainly used in


Optional: perhaps move this code below the switch statement and eliminate default case? I think Clang can produce a warning if a switch statement is non-exhaustive, but that won't work in presence of default.

var-const · 2018-04-09T15:52:40Z

Firestore/core/src/firebase/firestore/model/precondition.h

+#if defined(__OBJC__)
+  // Objective-C requires a default constructor.
+  Precondition()
+      : type_(Type::None),


Optional: since there is an in-class initializer, this line is unnecessary. Consider also providing in-class initializers for the other member variables. That way, the constructor could be just Precondition() {}.

I feel our usage of initializer is not consistent in this way. I don't see the heavy use of in-class initializer in other classes in our code base. The only exception of initializer to type_ is a safe mechanism to ensure type_ is always initialized intentionally. type_ is a particular member that ensure the integrity of the class (if the language support RTTI, type_ is not needed.), which is different from the other members.

In general, I think in-class initializers are superior to doing initialization in constructors (when applicable). They potentially reduce duplication (when the same member variable gets set to the same value across constructors), and, importantly, prevent forgetting to initialize a member variable when adding a new constructor. C++ core guidelines provide some recommendations to prefer in-class initializers: "Don't define a default constructor that only initializes data members", "Prefer in-class initializers to member initializers in constructors for constant initializers". This is of course non-binding, but I feel it's a good practice.

This is optional; I understand your reasoning, just wanted to provide my perspective on this.

var-const · 2018-04-09T15:53:25Z

Firestore/core/src/firebase/firestore/model/precondition.h

+        return [maybe_doc isKindOfClass:[FSTDocument class]] &&
+               firebase::firestore::model::SnapshotVersion(maybe_doc.version) ==
+                   update_time_;
+        break;


Same comment re. breaks.

var-const · 2018-04-09T15:56:51Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+    case Type::UpdateTime:
+      return maybe_doc.type() == MaybeDocument::Type::Document &&
+             maybe_doc.version() == update_time_;
+      break;


(similar comment re. break)

ok, removed since both of you are in favor of without break.

var-const · 2018-04-09T15:57:37Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+
+#include "Firestore/core/src/firebase/firestore/model/precondition.h"
+
+#include <utility>


Nit: <utility> is already included in the header file.

Done. But frankly speaking, I feel IWYU asking for include <utility> here.

NB: there's an exception for IWYU if the related header includes it.

"However, any includes present in the related header do not need to be included again in the related cc"

http://go/cpp-style#Names_and_Order_of_Includes

(I don't think it's a requirement to not include it though.)

var-const · 2018-04-09T16:00:12Z

Firestore/core/src/firebase/firestore/model/precondition.h

+
+#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"
+#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
+#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"


Nit: #include "Firestore/core/include/firebase/firestore/timestamp.h";?

var-const · 2018-04-09T16:06:06Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

+// SDKs in other platform.
+inline model::SnapshotVersion Version(int64_t version) {
+  return model::SnapshotVersion{
+      Timestamp{/*seconds=*/version / 1000,


If time is in microseconds, should this be version / (1000 * 1000)?

Also, optionally consider something like:

namespace chr = std::chrono; auto timepoint = chr::time_point<chr::system_clock>(chr::microseconds(version)); return model::SnapshotVersion{Timestamp::FromTimePoint(timepoint)};

This is a little more verbose, but arguably more readable and saves doing arithmetic operations manually.

var-const · 2018-04-09T16:15:44Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

+inline model::SnapshotVersion Version(int64_t version) {
+  return model::SnapshotVersion{
+      Timestamp{/*seconds=*/version / 1000,
+                /*nanoseconds=*/static_cast<int32_t>(version % 1000) * 1000}};


Similarly, should this be (version % (1000 * 1000)) * 1000? Or is version in milliseconds?

ack, should be 1000000 instead of 1000. change to your suggested implementation.

zxu123

Updated. PTAL

zxu123 · 2018-04-10T13:42:32Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+
+#include "Firestore/core/src/firebase/firestore/model/precondition.h"
+
+#include <utility>


Done. But frankly speaking, I feel IWYU asking for include <utility> here.

zxu123 · 2018-04-10T13:51:28Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+    case Type::UpdateTime:
+      return maybe_doc.type() == MaybeDocument::Type::Document &&
+             maybe_doc.version() == update_time_;
+      break;


ok, removed since both of you are in favor of without break.

zxu123 · 2018-04-10T13:53:09Z

Firestore/core/src/firebase/firestore/model/precondition.h

+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_PRECONDITION_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_PRECONDITION_H_
+
+#include <memory>


zxu123 · 2018-04-10T13:54:26Z

Firestore/core/src/firebase/firestore/model/precondition.h

+
+#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"
+#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
+#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"


zxu123 · 2018-04-10T14:02:08Z

Firestore/core/src/firebase/firestore/model/precondition.h

+#if defined(__OBJC__)
+  // Objective-C requires a default constructor.
+  Precondition()
+      : type_(Type::None),


I feel our usage of initializer is not consistent in this way. I don't see the heavy use of in-class initializer in other classes in our code base. The only exception of initializer to type_ is a safe mechanism to ensure type_ is always initialized intentionally. type_ is a particular member that ensure the integrity of the class (if the language support RTTI, type_ is not needed.), which is different from the other members.

zxu123 · 2018-04-10T14:07:30Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    switch (type_) {
+      case Type::None:
+        return @"<Precondition <none>>";
+        break;


Done. Really adding break do no harm but would catch bugs if later non-return code is added into the case. But I don't have strong opinion and thus removed.

zxu123 · 2018-04-10T14:14:37Z

Firestore/core/src/firebase/firestore/model/precondition.cc

+      }
+      break;
+    case Type::None:
+      FIREBASE_ASSERT_MESSAGE(IsNone(), "Precondition should be empty");


zxu123 · 2018-04-10T14:16:39Z

Firestore/core/src/firebase/firestore/model/precondition.h

+                             update_time_.timestamp().ToString().c_str()];
+        break;
+      default:
+        // We only raise dev assertion here. This function is mainly used in


zxu123 · 2018-04-10T14:18:16Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

+// SDKs in other platform.
+inline model::SnapshotVersion Version(int64_t version) {
+  return model::SnapshotVersion{
+      Timestamp{/*seconds=*/version / 1000,


zxu123 · 2018-04-10T14:21:12Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

+inline model::SnapshotVersion Version(int64_t version) {
+  return model::SnapshotVersion{
+      Timestamp{/*seconds=*/version / 1000,
+                /*nanoseconds=*/static_cast<int32_t>(version % 1000) * 1000}};


ack, should be 1000000 instead of 1000. change to your suggested implementation.

var-const · 2018-04-10T17:10:11Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

@@ -17,7 +17,8 @@
 #ifndef FIRESTORE_CORE_TEST_FIREBASE_FIRESTORE_TESTUTIL_TESTUTIL_H_
 #define FIRESTORE_CORE_TEST_FIREBASE_FIRESTORE_TESTUTIL_TESTUTIL_H_

-#include <inttypes.h>
+#include <chrono>  // NOLINT(build/c++11)
+#include <cinttypes>


Nit: please replace <cinttypes> with <cstdint> (IIUC, you're only including it for int64_t, and the header defining that is <cstdint>, while <cinttypes> happens to include it).

var-const · 2018-04-10T17:12:20Z

Firestore/core/src/firebase/firestore/model/precondition.h

+    switch (type_) {
+      case Type::None:
+        return @"<Precondition <none>>";
+        break;


I see your reasoning, thanks for explaining. I'm still leaning towards not having non-reachable code (and linters might complain).

var-const · 2018-04-10T20:25:28Z

Firestore/core/src/firebase/firestore/model/precondition.h

+#if defined(__OBJC__)
+  // Objective-C requires a default constructor.
+  Precondition()
+      : type_(Type::None),


In general, I think in-class initializers are superior to doing initialization in constructors (when applicable). They potentially reduce duplication (when the same member variable gets set to the same value across constructors), and, importantly, prevent forgetting to initialize a member variable when adding a new constructor. C++ core guidelines provide some recommendations to prefer in-class initializers: "Don't define a default constructor that only initializes data members", "Prefer in-class initializers to member initializers in constructors for constant initializers". This is of course non-binding, but I feel it's a good practice.

This is optional; I understand your reasoning, just wanted to provide my perspective on this.

wilhuff

Pretty much LGTM, with a few minor last points.

wilhuff · 2018-04-12T16:52:28Z

Firestore/core/src/firebase/firestore/model/snapshot_version.h

@@ -38,6 +43,27 @@ class SnapshotVersion {
  /** Creates a new version that is smaller than all other versions. */
  static const SnapshotVersion& None();

+#if defined(__OBJC__)
+  SnapshotVersion(FSTSnapshotVersion* version) {  // NOLINT(runtime/explicit)
+    if (![version isEqual:[FSTSnapshotVersion noVersion]]) {


Is this check even required? Why not just directly initialize the timestamp?

SnapshotVersion(FSTSnapshotVersion* version) : timestamp_{version.timestamp} { }

Or if Timestamp doesn't define a conversion to FIRTimestamp:

SnapshotVersion(FSTSnapshotVersion* version) : timestamp_{version.timestamp.seconds, version.timestamp.nanoseconds} { }

Done. not really required as far as the logic of noVersion are the same between objc and C++, which seems the case as both are of epoch 0.

wilhuff · 2018-04-12T16:55:26Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

@@ -40,6 +47,25 @@ inline model::ResourcePath Resource(absl::string_view field) {
  return model::ResourcePath::FromString(field);
 }

+// Version is epoch time in microseconds. This helper is defined so to match


"Version" here is ambiguous (it's not clear if you mean the function or the param.

Better to define this just in terms of what it does:

// Creates a snapshot version from the given version timestamp // // @param version a timestamp in microseconds since the epoch

wilhuff · 2018-04-12T16:58:05Z

Firestore/Source/Remote/FSTSerializerBeta.mm

-    message.exists = precondition.exists == FSTPreconditionExistsYes;
+  if (precondition.type() == Precondition::Type::UpdateTime) {
+    message.updateTime =
+        [self encodedVersion:static_cast<FSTSnapshotVersion *>(precondition.update_time())];


Is this cast necessary? -encodedVersion: statically takes an FSTSnapshotVersion so the compiler should find the user-defined conversion automatically, no?

zxu123

updated

zxu123 · 2018-04-12T19:23:38Z

Firestore/core/src/firebase/firestore/model/snapshot_version.h

@@ -38,6 +43,27 @@ class SnapshotVersion {
  /** Creates a new version that is smaller than all other versions. */
  static const SnapshotVersion& None();

+#if defined(__OBJC__)
+  SnapshotVersion(FSTSnapshotVersion* version) {  // NOLINT(runtime/explicit)
+    if (![version isEqual:[FSTSnapshotVersion noVersion]]) {


Done. not really required as far as the logic of noVersion are the same between objc and C++, which seems the case as both are of epoch 0.

zxu123 · 2018-04-12T19:28:37Z

Firestore/core/test/firebase/firestore/testutil/testutil.h

@@ -40,6 +47,25 @@ inline model::ResourcePath Resource(absl::string_view field) {
  return model::ResourcePath::FromString(field);
 }

+// Version is epoch time in microseconds. This helper is defined so to match


zxu123 · 2018-04-12T19:34:52Z

Firestore/Source/Remote/FSTSerializerBeta.mm

-    message.exists = precondition.exists == FSTPreconditionExistsYes;
+  if (precondition.type() == Precondition::Type::UpdateTime) {
+    message.updateTime =
+        [self encodedVersion:static_cast<FSTSnapshotVersion *>(precondition.update_time())];


wilhuff

LGTM

* port FieldMask to C++ * address changes * address changes * fix test * address change * Port transform operations (FSTTransformOperation, FSTServerTimestampTransform) to C++ * address changes * address changes * address changes * implement `FieldTransform` in C++ * port `FieldTransform` * make `fieldTransforms` shared inside `context` * Implement Precondition in C++ w/o test yet * add unit test for `Precondition` * port `Precondition` * address changes * address changes * fix bugs for integration test * address changes * fix lint * address changes * address changes

zxu123 added 17 commits March 29, 2018 15:59

port FieldMask to C++

96a64b3

address changes

c6ee4b0

address changes

64cfaac

fix test

d27dc11

address change

235bc20

Port transform operations (FSTTransformOperation, FSTServerTimestampT…

a97ef14

…ransform) to C++

address changes

6d47fe4

address changes

9ea9ab2

address changes

f25eebc

Merge branch 'master' into cpp/port_transform

a579ba5

implement FieldTransform in C++

8eecd35

port FieldTransform

b4c78da

Merge branch 'master' into cpp/port_field_transform

84e85d4

make fieldTransforms shared inside context

78a8bb9

Implement Precondition in C++ w/o test yet

f7bcff7

add unit test for Precondition

07d297c

port Precondition

d4df4b3

googlebot added the cla: yes label Apr 6, 2018

zxu123 requested review from var-const and rsgowman April 6, 2018 19:17

zxu123 added the api: firestore label Apr 6, 2018

address changes

a87cb0a

rsgowman reviewed Apr 6, 2018

View reviewed changes

zxu123 added 2 commits April 9, 2018 09:51

address changes

7936f8b

Merge branch 'master' into cpp/port_precondition

46bd043

googlebot added cla: no and removed cla: yes labels Apr 9, 2018

Merge branch 'master' into cpp/port_field_transform

0991762

Merge branch 'cpp/port_field_transform' into cpp/port_precondition

53af7c9

zxu123 commented Apr 9, 2018

View reviewed changes

rsgowman approved these changes Apr 9, 2018

View reviewed changes

fix bugs for integration test

6969212

var-const reviewed Apr 9, 2018

View reviewed changes

zxu123 added 2 commits April 10, 2018 09:38

Merge branch 'master' into cpp/port_field_transform

93cf65a

address changes

f15922a

zxu123 commented Apr 10, 2018

View reviewed changes

zxu123 added 2 commits April 10, 2018 12:37

fix lint

e6edf9a

Merge branch 'cpp/port_field_transform' into cpp/port_precondition

2e78fd6

var-const approved these changes Apr 10, 2018

View reviewed changes

zxu123 changed the base branch from cpp/port_field_transform to master April 10, 2018 20:29

Merge branch 'master' into cpp/port_precondition

dfadc12

zxu123 requested a review from wilhuff April 10, 2018 20:36

address changes

ff7703b

wilhuff reviewed Apr 12, 2018

View reviewed changes

address changes

fe1f777

zxu123 commented Apr 12, 2018

View reviewed changes

wilhuff approved these changes Apr 12, 2018

View reviewed changes

zxu123 merged commit 2ceea9b into master Apr 13, 2018

zxu123 deleted the cpp/port_precondition branch April 13, 2018 01:32

firebase locked and limited conversation to collaborators Nov 5, 2019


		#include "Firestore/core/src/firebase/firestore/model/precondition.h"

		#include <utility>

Port Precondition to C++ #1040

Port Precondition to C++ #1040

Conversation

zxu123 commented Apr 6, 2018 • edited Loading

Discussion

Testing

API Changes

zxu123 commented Apr 6, 2018

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

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

googlebot commented Apr 9, 2018

zxu123 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

Choose a reason for hiding this comment

rsgowman left a 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

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

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

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

Choose a reason for hiding this comment

wilhuff left a comment

Port `Precondition` to C++ #1040

Port `Precondition` to C++ #1040

zxu123 commented Apr 6, 2018 •

edited

Loading