Re: [PATCH 2/2] Introduce Python testcases to check DWARF output

[Pierre-Marie de Rodat <derodat@adacore.com>]

On 07/26/2017 07:09 PM, David Malcolm wrote:
>> +    If `single_cu` is True, make sure there is exactly one
>> compilation unit and
> 
> "is True" -> "is true"

Fixed.

>> +        :param bool or_error: When True, if `single` is True and no
>> attribute
> 
> "True" -> "true" in two places

Fixed.

>> +        :param None|(DIE) -> bool predicate: If provided, function
>> that filters
>> +            out DIEs when it returns False.

You did not suggested, but I replaced “False” with “false” to be 
consistent. ;-)

>> +        :param bool single: If True, look for a single DIE and raise
>> a
> 
> "True" -> "true", I suppose

Fixed.

>> +        If left to None, the match succeded. Otherwise, must be set
> 
> 
> "succeded" -> "succeeded"

Fixed.

>> +        This is valid iff the match succeded.
> 
> here again.

Likewise.

>> +    In Python 2, just return the input. In Python 3, decode the
>> input as ASCII.
>> +    """
>> +    return (str_or_byte if sys.version_info.major < 3 else
>> +            str_or_byte.decode('ascii'))
> 
> Aha!  Python 2 and Python 3.
> 
> 
> Presumably this all runs with LANG=C so that there's no danger of any
> non-ASCII bytes?  (bytes.decode('ascii' will raise a UnicodeDecodeError
> if any byte >=128).

I’m not sure about the interaction with the locale. What I thought was: 
I’ve never seen non-ASCII strings in DWARF, nor in objdump’s output. I 
know it’s theorically possible: if that happens in the future (like some 
language allows non-ASCII identifier and yield non-ASCII names in 
DWARF), we’ll only have this function to fix.

> There's a fair amount of non-trivial parsing going on here.
> I wonder if it would be helpful to add a "unittest" suite for the
> parsing?
> (e.g. to have some precanned fragments of objdump output as strings,
> and to verify that they're parsed as expected).
> 
> Note that I'm not a reviewer for the testsuite, so this is just a
> suggestion.

That’s a good idea. Actually I think it will be very easy to write such 
tests *and* to assess Python code coverage for them. I’ll do this if 
this proposal is in good way to be accepted.

> Hope this is constructive

It totally was: thank you very much!

-- 
Pierre-Marie de Rodat