[Webkit-unassigned] [Bug 119446] [GTK] Parse Valgrind xml leak files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119446





--- Comment #5 from Mario Sanchez Prada <mario at webkit.org>  2013-08-04 17:13:29 PST ---
(From update of attachment 208024)
View in context: https://bugs.webkit.org/attachment.cgi?id=208024&action=review

Quick informal review. Mostly coding style issues and some small issues

> Tools/Scripts/webkitpy/port/common.py:37
> +import logging
> +import platform
> +import os
> +import signal
> +import subprocess
> +import sys
> +import time

Please use alphabetical order

> Tools/Scripts/webkitpy/port/common.py:39
> +

Extra line

> Tools/Scripts/webkitpy/port/common.py:43
> +
> +

Two empty lines here, while one would be enough between definitions. Please fix all across the file (it happens all the time in this patch)

> Tools/Scripts/webkitpy/port/common.py:55
> +    """ Runs a subprocess, until it finishes or |timeout| is exceeded and the

Extra space before "Runs"

> Tools/Scripts/webkitpy/port/common.py:266
> +    """ Prints out the list of used suppressions in a format common to all the

Extra space after the """

> Tools/Scripts/webkitpy/port/gdb_helper.py:40
> +

Extra line here too

> Tools/Scripts/webkitpy/port/gdb_helper.py:42
> +    ''' Parse the gdb output line, return a pair (file, line num) '''

You use ''' instead of """ in this file for documenting functions. Please use """ always (to be consistent), avoid the extra space at the beginning and the end, and finish the sentence with a period

> Tools/Scripts/webkitpy/port/gdb_helper.py:49
> +
> +

As in the other file, please use one empty line only to mark separation between defined functions

> Tools/Scripts/webkitpy/port/gdb_helper.py:51
> +    ''' For each address, return a pair (file, line num) '''

Use """ and period at the end, and avoid the extra spaces for consistency. Please fix further functions in this file too

> Tools/Scripts/webkitpy/port/gtk.py:133
> +        # FIXME: This will include too many leaks in subsequent runs until the results directory is cleared!

Why we don't just clear the directory automatically each time this logic is run, right at the start?

That way, you would have the results available after one execution as long as you haven't triggered a new one, giving you time enough to copy them to a safe location in case you want to keep them

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:35
> +

Extra line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:47
> +        self._memcheck_leak_analyzer = MemcheckAnalyzer(port.webkit_base(),
> +                                                        use_gdb=False)

You can make this a single line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:54
> +        return self._filesystem.glob(self._filesystem.join(directory,
> +                                                           "*-leaks.xml"))

Likewise

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:63
> +

Extra line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:75
> +

Extra line. Please fix the rest like this one in this file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:94
> +    prefixes_to_cut = ["build/src/", "valgrind/coregrind/",
> +                       "out/Release/../../"]

This can be made just one line, as in the original version of the file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:140
> +            # Try using gdb

Missing period at the end

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:245
> +                self._backtraces.append([description,
> +                                         gatherFrames(node, source_dir)])

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:282
> +                buf += (frame[FUNCTION_NAME] or \
> +                        frame[INSTRUCTION_POINTER]) + "\n"

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:296
> +                    foo = TheAddressTable.GetFileLine(
> +                        frame[OBJECT_FILE],
> +                        frame[INSTRUCTION_POINTER])

Make this one line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:302
> +                    output += (" (" + frame[SRC_FILE_DIR] +
> +                               "/" + frame[SRC_FILE_NAME] +
> +                               ":" + frame[SRC_LINE] + ")")

These three lines can be only one too

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:323
> +        # Widen suppression slightly to make portable between mac and linux
> +        # TODO(timurrrr): Oops, these transformations should happen
> +        # BEFORE calculating the hash!

Missing periods at the end of sentences

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:330
> +        # Split into lines so we can enforce length limits

Missing period

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:406
> +            if prev_line.strip() in ["</error>",
> +                                     "</errorcounts>",
> +                                     "</status>"]:

This can be one line too

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.