Dexdiffer scripts

Dexdiffer scripts

Tools to allow us to diff between similiar dex files (using dextra to put them
into a readable form). This helps when checking proguard configs to see what
they are actually doing.

BUG=619124
Committed: https://crrev.com/b6dc58c6ce55c6691e21cd176983823842f70fef
Cr-Commit-Position: refs/heads/master@{#399568}

[smaier, 2016-06-10 19:33:23]

smaier@chromium.org changed reviewers:
+ agrieve@chromium.org

[agrieve, 2016-06-13 16:57:01]

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
File tools/android/dexdiffer/dexdiffer.py (right):

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:16: 
Two blank lines between top-level functions

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:17: def is_new_class(line):
naming convention is def _IsNewClass()

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:29: if line.endswith(':'):
nit: line.rstrip(':')

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:38: original_name, new_name =
split_string[0], split_string[1]
original_name, new_name = line.split(' -> ')

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:41: if original_name.find(' ') >= 0:
if ' ' in original_name, or cache result in a variable to not have to call
find() again within the condition

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:45: if original_name.find('(') and
original_name.find(')'):
this evaluates to true when no brackets exist. Might make sense to use re.search
for these:

match = re.search(r'\((.*?)\)', original_name)
if match:
  arguments_string = match.groups(1)

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:51: def read_mapping_dict(mappingFile):
mappingFile->mapping_file

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:79: def strip_quotes(string):
nit: might be more readable to use re here:
  return re.sub(r'([\'"]).*?\1', '', string)

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:96: qualifiers = ['public', 'protected',
'private', 'final', 'static', 'abstract',
nit: make this a set and put it at the top of the file:

_QUALIFIERS = set([...])

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:124: postfix =
renamed_type.replace(renamed_type_stripped, '')
postfix -> suffix

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:152: if function_args.endswith(','):
rstrip()

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:228: if
(base_class_members_set.issubset(new_class_members_set)
could you just say: base_class_members_set == new_class_members_set

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:235: if
base_class_members_set.difference(new_class_members_set):
nit: store in a variable.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:258: assert(len(unused_argv) == 4)
this doesn't seem unused :P

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:260: with open(unused_argv[0], "r") as f:
You should at least have a usage text and support --help.

Easiest way is to use argparse module.

[smaier, 2016-06-13 18:57:17]

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
File tools/android/dexdiffer/dexdiffer.py (right):

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:16: 
On 2016/06/13 16:57:01, agrieve wrote:
> Two blank lines between top-level functions

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:17: def is_new_class(line):
On 2016/06/13 16:57:01, agrieve wrote:
> naming convention is def _IsNewClass()

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:29: if line.endswith(':'):
On 2016/06/13 16:57:01, agrieve wrote:
> nit: line.rstrip(':')

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:38: original_name, new_name =
split_string[0], split_string[1]
On 2016/06/13 16:57:01, agrieve wrote:
> original_name, new_name = line.split(' -> ')

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:41: if original_name.find(' ') >= 0:
On 2016/06/13 16:57:01, agrieve wrote:
> if ' ' in original_name, or cache result in a variable to not have to call
> find() again within the condition

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:45: if original_name.find('(') and
original_name.find(')'):
On 2016/06/13 16:57:01, agrieve wrote:
> this evaluates to true when no brackets exist. Might make sense to use
re.search
> for these:
> 
> match = re.search(r'\((.*?)\)', original_name)
> if match:
>   arguments_string = match.groups(1)

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:51: def read_mapping_dict(mappingFile):
On 2016/06/13 16:57:01, agrieve wrote:
> mappingFile->mapping_file

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:79: def strip_quotes(string):
On 2016/06/13 16:57:00, agrieve wrote:
> nit: might be more readable to use re here:
>   return re.sub(r'([\'"]).*?\1', '', string)

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:96: qualifiers = ['public', 'protected',
'private', 'final', 'static', 'abstract',
On 2016/06/13 16:57:01, agrieve wrote:
> nit: make this a set and put it at the top of the file:
> 
> _QUALIFIERS = set([...])

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:124: postfix =
renamed_type.replace(renamed_type_stripped, '')
On 2016/06/13 16:57:01, agrieve wrote:
> postfix -> suffix

Typically modifiers/operators after a token are referred to as "postfix"

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:152: if function_args.endswith(','):
On 2016/06/13 16:57:01, agrieve wrote:
> rstrip()

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:228: if
(base_class_members_set.issubset(new_class_members_set)
On 2016/06/13 16:57:01, agrieve wrote:
> could you just say: base_class_members_set == new_class_members_set

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:235: if
base_class_members_set.difference(new_class_members_set):
On 2016/06/13 16:57:01, agrieve wrote:
> nit: store in a variable.

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:258: assert(len(unused_argv) == 4)
On 2016/06/13 16:57:00, agrieve wrote:
> this doesn't seem unused :P

Done.

https://codereview.chromium.org/2057323002/diff/1/tools/android/dexdiffer/dex...
tools/android/dexdiffer/dexdiffer.py:260: with open(unused_argv[0], "r") as f:
On 2016/06/13 16:57:00, agrieve wrote:
> You should at least have a usage text and support --help.
> 
> Easiest way is to use argparse module.

Done.

[agrieve, 2016-06-13 20:17:33]

a couple more nits, but lgtm overall.

https://codereview.chromium.org/2057323002/diff/20001/tools/android/dexdiffer...
File tools/android/dexdiffer/dexdiffer.py (right):

https://codereview.chromium.org/2057323002/diff/20001/tools/android/dexdiffer...
tools/android/dexdiffer/dexdiffer.py:128: is_function):
nit: indent

https://codereview.chromium.org/2057323002/diff/20001/tools/android/dexdiffer...
tools/android/dexdiffer/dexdiffer.py:259: with open(args.base_mapping_file, "r")
as f:
nit: single quotes. Although "r" is the default anyways, so you could just
remove the param

[agrieve, 2016-06-13 20:18:07]

agrieve@chromium.org changed reviewers:
+ michaelbai@chromium.org

[agrieve, 2016-06-13 20:18:07]

michaelbai for OWNERS

[michaelbai, 2016-06-13 20:26:57]

LGTM, rubber stamp

[smaier, 2016-06-13 21:00:46]

The CQ bit was checked by smaier@chromium.org

[smaier, 2016-06-13 21:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2057323002/#ps40001
(title: "Few nits")

[commit-bot: I haz the power, 2016-06-13 21:03:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2057323002/40001

[commit-bot: I haz the power, 2016-06-13 22:14:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-13 22:15:04]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 22:17:16]

Description was changed from

==========
Dexdiffer scripts

Tools to allow us to diff between similiar dex files (using dextra to put them
into a readable form). This helps when checking proguard configs to see what
they are actually doing.

BUG=619124
==========

to

==========
Dexdiffer scripts

Tools to allow us to diff between similiar dex files (using dextra to put them
into a readable form). This helps when checking proguard configs to see what
they are actually doing.

BUG=619124

Committed: https://crrev.com/b6dc58c6ce55c6691e21cd176983823842f70fef
Cr-Commit-Position: refs/heads/master@{#399568}
==========

[commit-bot: I haz the power, 2016-06-13 22:17:18]

Patchset 3 (id:??) landed as
https://crrev.com/b6dc58c6ce55c6691e21cd176983823842f70fef
Cr-Commit-Position: refs/heads/master@{#399568}