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..67ee9f1df90b4eb6b73ded8fa739160496f6a8d2 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"
 
@@ -519,11 +520,30 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
 
   // Converts an exploded structure representing either the local time or UTC
   // into a Time class.
+  // TODO(maksims): Get rid of these in favor of the methods below when
+  // all the callers stop using these ones.
   static Time FromUTCExploded(const Exploded& exploded) {
-    return FromExploded(false, exploded);
+    base::Time time;
+    ignore_result(FromExploded(false, exploded, &time));

[Mark Mentovai, 2016/05/26 14:08:39]

Centralize the “false”/“true”. This should be
ignore_result(FromUTCExploded(exploded, &time)). Similar on line 532.

Rationale: “false”/“true” require reading the definition to understand whether
it’s “is_local” or “is_utc” or something else entirely. Since it’s a private
implementation detail, it’s not worth adding an enum to clarify this at call
sites, but since equivalent function names are available, you should use them.

+    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. Returns false on failure. For example,
+  // 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.

[Mark Mentovai, 2016/05/26 14:08:39]

Nuke “which results in 1st day of the next month.” We’re not defining what the
31st day of a 30-day month means, we’re saying that it’s a failure.

+  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 +584,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:-
@@ -577,6 +601,9 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
   static bool FromStringInternal(const char* time_string,
                                  bool is_local,
                                  Time* parsed_time);
+
+  // Comparison does not consider |day_of_week| when doing the operation.
+  static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs);

[Mark Mentovai, 2016/05/26 14:08:39]

Not sure why this isn’t a non-static MostlyEquals() defined as a member of
Exploded. Is there a reason?

[mmenke, 2016/05/26 14:45:08]

If we put it there, we'd either need to make it public, which seems not great,
or make this class a friend of Exploded.

[maksims (do not use this acc), 2016/05/27 10:19:08]

Yes, exactly. Should I change it to be a friend then?

 };
 
 // static