Readdress use_counter_feature_enum issue

tools/metrics/histograms/update_histogram_enum.py

Index: tools/metrics/histograms/update_histogram_enum.py
diff --git a/tools/metrics/histograms/update_histogram_enum.py b/tools/metrics/histograms/update_histogram_enum.py
index 7e4c584d81955ff8beb4f9c102181a1c4b5e0989..4ac3f2049c006967cc29b01161038cb66cd8ef48 100644
--- a/tools/metrics/histograms/update_histogram_enum.py
+++ b/tools/metrics/histograms/update_histogram_enum.py
@@ -33,13 +33,24 @@ class UserError(Exception):
     return self.args[0]
 
 
+class DuplicatedValue(Exception):
+  """Exception raised for duplicated enum values.
+
+  Attributes:
+      first_label: First enum label that shares the duplicated enum value.
+      second_label: Second enum label that shares the duplicated enum value.
+  """
+  def __init__(self, first_label, second_label):
+    self.first_label = first_label
+    self.second_label = second_label
+
+
 def Log(message):
   logging.info(message)
 
 
 def ReadHistogramValues(filename, start_marker, end_marker, strip_k_prefix):
-  """Returns a dictionary of enum values and a pair of labels that have the same
-     enum values, read from a C++ file.
+  """Creates a dictionary of enum values, read from a C++ file.
 
   Args:
       filename: The unix-style path (relative to src/) of the file to open.
@@ -47,6 +58,12 @@ def ReadHistogramValues(filename, start_marker, end_marker, strip_k_prefix):
       end_marker: A regex that signifies the end of the enum values.
       strip_k_prefix: Set to True if enum values are declared as kFoo and the
           'k' should be stripped.
+
+  Returns:
+      A boolean indicating wheather the histograms.xml file would be changed.
+
+  Raises:
+      DuplicatedValue: An error when two enum labels share the same value.
   """
   # Read the file as a list of lines
   with open(path_util.GetInputFile(filename)) as f:
@@ -87,15 +104,15 @@ def ReadHistogramValues(filename, start_marker, end_marker, strip_k_prefix):
         label = m.group(1)
       else:
         continue
+    # If two enum labels have the same value
+    if enum_value in result:
+      raise DuplicatedValue(result[enum_value], label)

[Ilya Sherman, 2017/04/28 20:59:49]

Please keep this after strip_k_prefix -- the order was intentionally changed in
crrev.com/2841823007

[lunalu1, 2017/05/01 18:20:44]

That makes sense. Sorry for the mistake.
Done.

     if strip_k_prefix:
       assert label.startswith('k'), "Enum " + label + " should start with 'k'."
       label = label[1:]
-    # If two enum labels have the same value
-    if enum_value in result:
-      return result, (result[enum_value], label)
     result[enum_value] = label
     enum_value += 1
-  return result, None
+  return result
 
 
 def CreateEnumItemNode(document, value, label):
@@ -202,14 +219,14 @@ def CheckPresubmitErrors(histogram_enum_name, update_script_name,
       A string with presubmit failure description, or None (if no failures).
   """
   Log('Reading histogram enum definition from "{0}".'.format(source_enum_path))
-  source_enum_values, duplicated_values = ReadHistogramValues(
-      source_enum_path, start_marker, end_marker, strip_k_prefix)
-
-  if duplicated_values:
+  try:
+    source_enum_values = ReadHistogramValues(
+        source_enum_path, start_marker, end_marker, strip_k_prefix)
+  except DuplicatedValue as duplicated_values:
     return ('%s enum has been updated and there exist '
-            'duplicated values between (%s) and (%s)' % (histogram_enum_name,
-                                                         duplicated_values[0],
-                                                         duplicated_values[1]))
+            'duplicated values between (%s) and (%s)' %
+            (histogram_enum_name, duplicated_values.first_label,
+             duplicated_values.second_label))
 
   (xml, new_xml) = _GetOldAndUpdatedXml(histogram_enum_name, source_enum_values,
                                         source_enum_path)
@@ -218,8 +235,6 @@ def CheckPresubmitErrors(histogram_enum_name, update_script_name,
             'regenerated. Please run %s in src/tools/metrics/histograms/ to '
             'update the mapping.' % (histogram_enum_name, update_script_name))
 
-  return None

[Ilya Sherman, 2017/04/28 20:59:49]

Why did you remove this return stmt?

[lunalu1, 2017/05/01 18:20:44]

My bad, I thought it will just return None by default.
Done.

-
 
 def UpdateHistogramFromDict(histogram_enum_name, source_enum_values,
                             source_enum_path):