From: Hans Fugal <fugalh@fb.com>
Date: Thu, 2 Jul 2015 01:18:53 +0000 (-0700)
Subject: Discourage Duration in code comments and tests
X-Git-Tag: v0.49.0~10
X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=83e4f1b3b7217806c249c32d0fd26eab15604e6b;p=folly.git

Discourage Duration in code comments and tests

Reviewed By: @yfeldblum

Differential Revision: D2209095
---

diff --git a/folly/futures/Timekeeper.h b/folly/futures/Timekeeper.h
index 3e3e20a7..024b9eb7 100644
--- a/folly/futures/Timekeeper.h
+++ b/folly/futures/Timekeeper.h
@@ -41,7 +41,11 @@ template <class> class Future;
 /// it made sense to introduce a cleaner term.
 ///
 /// Remember that Duration is a std::chrono duration (millisecond resolution
-/// at the time of writing).
+/// at the time of writing). When writing code that uses specific durations,
+/// prefer using the explicit std::chrono type, e.g. std::chrono::milliseconds
+/// over Duration. This makes the code more legible and means you won't be
+/// unpleasantly surprised if we redefine Duration to microseconds, or
+/// something.
 class Timekeeper {
  public:
   virtual ~Timekeeper() = default;
diff --git a/folly/futures/detail/Types.h b/folly/futures/detail/Types.h
index 53bd66a3..6f87b2fb 100644
--- a/folly/futures/detail/Types.h
+++ b/folly/futures/detail/Types.h
@@ -20,6 +20,19 @@
 
 namespace folly {
 
+/// folly::Duration is an alias for the best resolution we offer/work with.
+/// However, it is not intended to be used for client code - you should use a
+/// descriptive std::chrono::duration type instead. e.g. do not write this:
+///
+///   futures::sleep(Duration(1000))...
+///
+/// rather this:
+///
+///   futures::sleep(std::chrono::milliseconds(1000));
+///
+/// or this:
+///
+///   futures::sleep(std::chrono::seconds(1));
 using Duration = std::chrono::milliseconds;
 
 }
diff --git a/folly/futures/test/TimekeeperTest.cpp b/folly/futures/test/TimekeeperTest.cpp
index 4c500cb5..fdf147c5 100644
--- a/folly/futures/test/TimekeeperTest.cpp
+++ b/folly/futures/test/TimekeeperTest.cpp
@@ -21,7 +21,9 @@
 #include <unistd.h>
 
 using namespace folly;
+using std::chrono::milliseconds;
 
+std::chrono::milliseconds const zero_ms(0);
 std::chrono::milliseconds const one_ms(1);
 std::chrono::milliseconds const awhile(10);
 std::chrono::seconds const too_long(10);
@@ -39,8 +41,6 @@ struct TimekeeperFixture : public testing::Test {
 };
 
 TEST_F(TimekeeperFixture, after) {
-  Duration waited(0);
-
   auto t1 = now();
   auto f = timeLord_->after(awhile);
   EXPECT_FALSE(f.isReady());
@@ -72,7 +72,7 @@ TEST(Timekeeper, futureGetBeforeTimeout) {
 
 TEST(Timekeeper, futureGetTimeout) {
   Promise<int> p;
-  EXPECT_THROW(p.getFuture().get(Duration(1)), folly::TimedOut);
+  EXPECT_THROW(p.getFuture().get(one_ms), folly::TimedOut);
 }
 
 TEST(Timekeeper, futureSleep) {
@@ -131,7 +131,7 @@ TEST(Timekeeper, futureWithinException) {
 TEST(Timekeeper, onTimeout) {
   bool flag = false;
   makeFuture(42).delayed(one_ms)
-    .onTimeout(Duration(0), [&]{ flag = true; return -1; })
+    .onTimeout(zero_ms, [&]{ flag = true; return -1; })
     .get();
   EXPECT_TRUE(flag);
 }
@@ -139,17 +139,17 @@ TEST(Timekeeper, onTimeout) {
 TEST(Timekeeper, onTimeoutReturnsFuture) {
   bool flag = false;
   makeFuture(42).delayed(one_ms)
-    .onTimeout(Duration(0), [&]{ flag = true; return makeFuture(-1); })
+    .onTimeout(zero_ms, [&]{ flag = true; return makeFuture(-1); })
     .get();
   EXPECT_TRUE(flag);
 }
 
 TEST(Timekeeper, onTimeoutVoid) {
   makeFuture().delayed(one_ms)
-    .onTimeout(Duration(0), [&]{
+    .onTimeout(zero_ms, [&]{
      });
   makeFuture().delayed(one_ms)
-    .onTimeout(Duration(0), [&]{
+    .onTimeout(zero_ms, [&]{
        return makeFuture<Unit>(std::runtime_error("expected"));
      });
   // just testing compilation here
@@ -162,7 +162,7 @@ TEST(Timekeeper, interruptDoesntCrash) {
 
 TEST(Timekeeper, chainedInterruptTest) {
   bool test = false;
-  auto f = futures::sleep(Duration(100)).then([&](){
+  auto f = futures::sleep(milliseconds(100)).then([&](){
     test = true;
   });
   f.cancel();
@@ -182,16 +182,17 @@ TEST(Timekeeper, executor) {
 
   auto f = makeFuture();
   ExecutorTester tester;
-  f.via(&tester).within(std::chrono::milliseconds(1)).then([&](){}).wait();
+  f.via(&tester).within(one_ms).then([&](){}).wait();
   EXPECT_EQ(2, tester.count);
 }
+
 // TODO(5921764)
 /*
 TEST(Timekeeper, onTimeoutPropagates) {
   bool flag = false;
   EXPECT_THROW(
     makeFuture(42).delayed(one_ms)
-      .onTimeout(Duration(0), [&]{ flag = true; })
+      .onTimeout(zero_ms, [&]{ flag = true; })
       .get(),
     TimedOut);
   EXPECT_TRUE(flag);