Skip to content

Now offers a summary of the beam file and not the full thing - #2

Open
gordonguthrie wants to merge 2 commits into
aerosol:masterfrom
gordonguthrie:master
Open

Now offers a summary of the beam file and not the full thing#2
gordonguthrie wants to merge 2 commits into
aerosol:masterfrom
gordonguthrie:master

Conversation

@gordonguthrie

Copy link
Copy Markdown

See changes to the README for details

Gordon Guthrie added 2 commits December 29, 2017 11:57
@aerosol

aerosol commented Jan 24, 2018

Copy link
Copy Markdown
Owner

Hi @gordonguthrie this is cool, please bare with me, I'll get to it shortly

@aerosol aerosol left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions inline if you don't mind, mainly just style

Comment thread lib/decompilerl.ex
[]
end
defp format(behaviours, prefix) do
[h | t] = behaviours

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match in the function clause instead?

Comment thread README.md
This can be called as so:

```elixir
Decompilerl.summarise('_build/dev/lib/myapp/ebin/Elixir.MyApp.AuthController.beam')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's uncommon for elixir APIs to accept lists as "strings", since strings are binaries by default. What do you think?

Comment thread README.md
Decompilerl

usage: decompierl <beam_file> [-o <erl_file> | --output=<erl_file>]
usage: decompierl <beam_file> [-o <erl_file> | --output=<erl_file> | -s | --summary]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, there's a typo there: decompierl

Comment thread lib/decompilerl.ex
end

defp format_summary(map) do
%{:file => file,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it'd be more idiomatic to match (preferably in the function clause) like this:

%{file: file, module: module, ...}

Comment thread lib/decompilerl.ex
format(behaviours, "Behaviours : "),
format(exports, "Exported Fns : "),
get_private_functions(functions, exports)
|>format("Private Fns : ")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 nitpick: |> format

Comment thread lib/decompilerl.ex
lines = [
"File : " <> file,
"Module : " <> module,
format(behaviours, "Behaviours : "),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider using String.pad_trailing/3 for all the formatting touch ups?

Comment thread lib/decompilerl.ex
get_private_functions(functions, exports)
|>format("Private Fns : ")
]
Enum.join(List.flatten(lines), "\n")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to flatten, iolists are OK to write

Comment thread lib/decompilerl.ex
append_value(map, :behaviours, [Atom.to_string(behaviour)])
end

defp process_ast({:function, _, func, arity, _body}, map) do

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps glue the function clauses together, i.e. no white space in between?

Comment thread lib/decompilerl.ex
end

defp make_fn_declaration(func, arity) do
Atom.to_string(func) <> "/" <> Integer.to_string(arity)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"#{func}/#{arity}"

Comment thread lib/decompilerl.ex

defp append_value(map, key, valuelist) when is_list(valuelist) do
%{^key => values} = map
Map.put(map, key, values ++ valuelist)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could avoid matching and pinning the key with:

Map.update(map, key, xs, &(xs ++ &1))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants