Add: check exploded time is properly converted

base/time/time.h

Index: base/time/time.h
diff --git a/base/time/time.h b/base/time/time.h
index 399ec826ce375e2adeeb5c8b58b1a9aeef18edb8..0c25a95b147ae5f40dffca8c22a72812fb18964c 100644
--- a/base/time/time.h
+++ b/base/time/time.h
@@ -56,6 +56,7 @@
 #include <limits>
 
 #include "base/base_export.h"
+#include "base/compiler_specific.h"
 #include "base/numerics/safe_math.h"
 #include "build/build_config.h"
 
@@ -424,6 +425,7 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
     // respective ranges. A 'true' return value does not guarantee the
     // Exploded value can be successfully converted to a Time value.
     bool HasValidValues() const;
+    bool operator!=(const Exploded& rhs);

[Mark Mentovai, 2016/05/24 18:27:27]

Put a blank line before this, because it’s not related to HasValidValues().

Specify that this doesn’t actually consider day_of_week, unless you want it to,
in which case you should update the implementation.

[mmenke, 2016/05/24 18:31:18]

I'm not sure we can check day of week...So as I believe someone suggested
earlier, this probably should not be operator !=.

Maybe instead make a private comparison function in base::Time, so people don't
run into that weirdness, and call it something weird, like ExplodedMostlyEquals?
 If Mark disagrees, I defer to him on this.

[Mark Mentovai, 2016/05/24 18:34:13]

mmenke wrote:
Maybe it is better that we rename it, then.

[maksims (do not use this acc), 2016/05/26 04:38:18]

Done.

   };
 
   // Contains the NULL time. Use Time::Now() to get the current time.
@@ -520,10 +522,29 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
   // Converts an exploded structure representing either the local time or UTC
   // into a Time class.

[mmenke, 2016/05/24 16:08:33]

Should add a TODO here:

TODO(maksims):  Get rid of these in favor of the methods below.

Note that the TODO isn't a promise that you'll do it, just a pointer to who has
context (I think it's reasonable for you to take it on yourself in a followup
CL, and doesn't look too bad, just explaining how things work).

[maksims (do not use this acc), 2016/05/26 04:38:18]

Done.

   static Time FromUTCExploded(const Exploded& exploded) {
-    return FromExploded(false, exploded);
+    base::Time time;
+    ignore_result(FromExploded(false, exploded, &time));
+    return time;
   }
   static Time FromLocalExploded(const Exploded& exploded) {
-    return FromExploded(true, exploded);
+    base::Time time;
+    ignore_result(FromExploded(true, exploded, &time));
+    return time;
+  }
+
+  // Converts an exploded structure representing either the local time or UTC
+  // into a Time class. This is a transition to a new format, where the
+  // converted time is set to |time| and FromUTCExploded, FromLocalExploded and
+  // FromExploded return true on success or false on failure. For example,

[mmenke, 2016/05/24 16:08:33]

Think your second sentence shouldn't refer to the old methods.

Instead, should just replace the second sentence with "Returns false on failure.
 For example, ..."

[maksims (do not use this acc), 2016/05/26 04:38:18]

Done.

+  // FromExploded can fail when 31st of a month is set on a 28-30 day month,
+  // which results in 1st day of the next month.
+  static bool FromUTCExploded(const Exploded& exploded,
+                              Time* time) WARN_UNUSED_RESULT {
+    return FromExploded(false, exploded, time);
+  }
+  static bool FromLocalExploded(const Exploded& exploded,
+                                Time* time) WARN_UNUSED_RESULT {
+    return FromExploded(true, exploded, time);
   }
 
   // Converts a string representation of time to a Time object.
@@ -564,8 +585,12 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
   void Explode(bool is_local, Exploded* exploded) const;
 
   // Unexplodes a given time assuming the source is either local time
-  // |is_local = true| or UTC |is_local = false|.
-  static Time FromExploded(bool is_local, const Exploded& exploded);
+  // |is_local = true| or UTC |is_local = false|. Function returns false on
+  // failure and sets |time| to Time(0). Otherwise returns true and sets |time|
+  // to non-exploded time.
+  static bool FromExploded(bool is_local,
+                           const Exploded& exploded,
+                           Time* time) WARN_UNUSED_RESULT;
 
   // Converts a string representation of time to a Time object.
   // An example of a time string which is converted is as below:-