[Predator] Make ``CallStackDetector`` return a namedtuple.

appengine/findit/crash/callstack_detectors.py

Index: appengine/findit/crash/callstack_detectors.py
diff --git a/appengine/findit/crash/callstack_detectors.py b/appengine/findit/crash/callstack_detectors.py
index 6dc2cc60703c6d4ffd225280ae1c6a9ba7fb44e2..6bb4a1d0d7effe413858d05e71d9cb6cd27c5ab6 100644
--- a/appengine/findit/crash/callstack_detectors.py
+++ b/appengine/findit/crash/callstack_detectors.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from collections import namedtuple
 import re
 
 from crash.type_enums import CallStackFormatType
@@ -9,11 +10,32 @@ from crash.type_enums import LanguageType
 from crash.type_enums import SanitizerType
 
 
+class StartOfCallStack(
+    namedtuple('StartOfCallStack',

[wrengr, 2016/12/29 20:13:02]

I'd put this on the previous line, to match the style of all the other
namedtuples

[Sharu Jiang, 2016/12/29 21:40:14]

Actually that's a wrong indentation. Didn't notice before...
https://google.github.io/styleguide/pyguide.html#Indentation

Either:
       # Aligned with opening delimiter
       foo = long_function_name(var_one, var_two,
                                var_three, var_four)

Or:
       # 4-space hanging indent; nothing on first line
       foo = long_function_name(
           var_one, var_two, var_three,
           var_four)

+               ['priority', 'format_type', 'language_type', 'metadata'])):
+  """Represents the start of a new callstack.
+

[wrengr, 2016/12/29 20:13:02]

Shouldn't there be a header word here? like "Properties:" or "Attributes:" or
something?

[Sharu Jiang, 2016/12/29 21:40:14]

Done.

+  priority (int): Priority of the new callstack.
+  format_type (CallStackFormatType): The format of the new callstack.
+  language_type (LanguageType): The language of the new callstack.
+  metadata (dict): Dict of metadata for the new callstack, e.g. pid of the
+    stack.
+  """
+  __slots__ = ()
+

[wrengr, 2016/12/29 20:13:02]

Maybe add a __new__ constructor, so that the metatata can be made optional
(since most callers of the constructor pass the empty dict)

[Sharu Jiang, 2016/12/29 21:40:14]

Done.

+  def __str__(self): # pragma: no cover
+    return ('StartOfCallStack(priority = %d, format_type = %d, '

[wrengr, 2016/12/29 20:13:02]

Use '%s' and self.__class__.__name__ instead of repeating the class name; to DRY
and to make future refactoring easier.

[Sharu Jiang, 2016/12/29 21:40:14]

Done.

+            'language_type = %d, metadata = %s)' % (self.priority,
+                                                    self.format_type,
+                                                    self.language_type,
+                                                    self.metadata))
+
+
 class CallStackDetector(object):
   """Class for detecting the start of a particular sort of CallStack."""
 
   # TODO(crbug.com/677321) Make the return to be a namedtuple.

[wrengr, 2016/12/29 20:13:02]

the todo is redundant now

[Sharu Jiang, 2016/12/29 21:40:14]

Oops, should have cleaned this.

-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):

[wrengr, 2016/12/29 20:13:02]

+1 to using __call__ :)

     """Determines whether a line is the start of a new callstack or not.
 
     Args:
@@ -21,17 +43,7 @@ class CallStackDetector(object):
       flags (FlagManager): manager for keeping track of parsing flags.
 
     Returns:
-      A tuple -
-      (is_new_callstack, priority, format_type, language_type, metadata)
-      is_new_callstack (bool): Boolean indicating whether this is the start of
-        a new callstack.
-      priority (int): Priority of a callstack.
-      format_type (CallStackFormatType): The format of the new callstack, or
-        None if there is no new callstack.
-      language_type (LanguageType): The language of the new callstack, or None
-        if there is no new callstack.
-      metadata (dict): Dict of metadata for the new callstack, e.g. pid of
-        the stack.
+      A ``StartOfCallStack`` or None if no callstack found.
     """
     raise NotImplementedError()
 
@@ -50,17 +62,19 @@ class AndroidJobDetector(CallStackDetector):
                 JAVA_CAUSED_BY_CALLSTACK_START_PATTERN,
                 JAVA_ANDROID_CALLSTACK_START_PATTERN]))
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     if AndroidJobDetector.JAVA_CALLSTACK_START_REGEX.match(line):
       # Only assign the highest priority to fatal exception stack or segv
       # stack.
       if flags and flags.Get('java_main_stack_flag'):
         flags.TurnOff('java_main_stack_flag')
-        return True, 0, CallStackFormatType.JAVA, LanguageType.JAVA, {}
+        return StartOfCallStack(0, CallStackFormatType.JAVA,
+                                LanguageType.JAVA, {})
 
-      return True, 1, CallStackFormatType.JAVA, LanguageType.JAVA, {}
+      return StartOfCallStack(1, CallStackFormatType.JAVA,
+                              LanguageType.JAVA, {})
 
-    return False, None, None, None, None
+    return None
 
 
 class SyzyasanDetector(CallStackDetector):
@@ -68,16 +82,18 @@ class SyzyasanDetector(CallStackDetector):
   SYZYASAN_CRASH_CALLSTACK_START_REGEX = re.compile(r'^Crash stack:$')
   SYZYASAN_NON_CRASH_CALLSTACK_START_REGEX = re.compile(r'^(?!Crash).* stack:$')
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     # In syzyasan build, new stack starts with 'crash stack:',
     # 'freed stack:', etc.
     if SyzyasanDetector.SYZYASAN_CRASH_CALLSTACK_START_REGEX.match(line):
-      return True, 0, CallStackFormatType.SYZYASAN, LanguageType.CPP, {}
+      return StartOfCallStack(0, CallStackFormatType.SYZYASAN,
+                              LanguageType.CPP, {})
     # Other callstacks all get priority 1.
     if SyzyasanDetector.SYZYASAN_NON_CRASH_CALLSTACK_START_REGEX.match(line):
-      return True, 1, CallStackFormatType.SYZYASAN, LanguageType.CPP, {}
+      return StartOfCallStack(1, CallStackFormatType.SYZYASAN,
+                              LanguageType.CPP, {})
 
-    return False, None, None, None, None
+    return None
 
 
 class TsanDetector(CallStackDetector):
@@ -97,31 +113,35 @@ class TsanDetector(CallStackDetector):
       '|'.join([TSAN_ALLOCATION_CALLSTACK_START_PATTERN,
                 TSAN_LOCATION_CALLSTACK_START_PATTERN]))
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     # Crash stack gets priority 0.
     if TsanDetector.TSAN_CRASH_CALLSTACK_START_REGEX.match(line):
-      return True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
     # All other stacks get priority 1.
     if TsanDetector.TSAN_NON_CRASH_CALLSTACK_START_REGEX.match(line):
-      return True, 1, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(1, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
-    return False, None, None, None, None
+    return None
 
 
 class UbsanDetector(CallStackDetector):
   """Detects the start of an undefined-behavior callstack."""
   UBSAN_CALLSTACK_START_REGEX = re.compile(r'^.*: runtime error: .*$')
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     if UbsanDetector.UBSAN_CALLSTACK_START_REGEX.match(line):
       if flags and flags.Get('is_first_stack_flag'):
         flags.TurnOff('is_first_stack_flag')
-        return True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+        return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                                LanguageType.CPP, {})
 
-      return True, 1, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(1, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
-    return False, None, None, None, None
+    return None
 
 
 class MsanDetector(CallStackDetector):
@@ -130,19 +150,22 @@ class MsanDetector(CallStackDetector):
   MSAN_CREATION_CALLSTACK_START_MARKER = 'Uninitialized value was created by'
   MSAN_STORAGE_CALLSTACK_START_MARKER = 'Uninitialized value was stored to'
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     # Assign the only msan stack priority 0.
     if MsanDetector.MSAN_CREATION_CALLSTACK_START_MARKER in line:
-      return True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
     if MsanDetector.MSAN_STORAGE_CALLSTACK_START_MARKER in line:
-      return True, 1, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(1, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
     msan_callstack_start_regex = (
         MsanDetector.MSAN_CALLSTACK_START_REGEX.match(line))
     if msan_callstack_start_regex:
-      return (True, 2, CallStackFormatType.DEFAULT, LanguageType.CPP,
-              {'pid': int(msan_callstack_start_regex.group(1).strip())})
+      return StartOfCallStack(
+          2, CallStackFormatType.DEFAULT, LanguageType.CPP,
+          {'pid': int(msan_callstack_start_regex.group(1).strip())})
 
-    return False, None, None, None, None
+    return None
 
 
 class AsanDetector(CallStackDetector):
@@ -164,23 +187,26 @@ class AsanDetector(CallStackDetector):
                 ASAN_ALLOCATION_CALLSTACK_START_PATTERN,
                 ASAN_OTHER_CALLSTACK_START_PATTERN]))
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     asan_crash_callstack_start_regex1_match = (
         AsanDetector.ASAN_CRASH_CALLSTACK_START_REGEX1.match(line))
     if asan_crash_callstack_start_regex1_match:
-      return (True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP,
-              {'pid': int(
-                  asan_crash_callstack_start_regex1_match.group(1).strip())})
+      return StartOfCallStack(
+          0, CallStackFormatType.DEFAULT, LanguageType.CPP,
+          {'pid': int(
+              asan_crash_callstack_start_regex1_match.group(1).strip())})
 
     # Crash stack gets priority 0.
     if AsanDetector.ASAN_CRASH_CALLSTACK_START_REGEX2.match(line):
-      return True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
     # All other callstack gets priority 1.
     if AsanDetector.ASAN_NON_CRASH_CALLSTACK_START_PATTERN.match(line):
-      return True, 1, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(1, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
-    return False, None, None, None, None
+    return None
 
 
 class ChromeCrashStackDetector(CallStackDetector):
@@ -188,12 +214,14 @@ class ChromeCrashStackDetector(CallStackDetector):
   CHROME_CRASH_CALLSTACK_START_REGEX = re.compile(r'CRASHED \[(.*) @ 0x(.*)\]')
   JAVA_CALLSTACK_START_REGEX = re.compile(r'\(JAVA\) CRASHED \[(.*) @ 0x(.*)\]')
 
-  def IsStartOfNewCallStack(self, line, flags=None):
+  def __call__(self, line, flags=None):
     if ChromeCrashStackDetector.CHROME_CRASH_CALLSTACK_START_REGEX.match(line):
       # Fracas only provide magic signature stack (crash stack).
-      return True, 0, CallStackFormatType.DEFAULT, LanguageType.CPP, {}
+      return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                              LanguageType.CPP, {})
 
     if ChromeCrashStackDetector.JAVA_CALLSTACK_START_REGEX.match(line):
-      return True, 0, CallStackFormatType.DEFAULT, LanguageType.JAVA, {}
+      return StartOfCallStack(0, CallStackFormatType.DEFAULT,
+                              LanguageType.JAVA, {})
 
-    return False, None, None, None, None
+    return None