Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdlib: make it possible to list local functions and types #7844

Conversation

frazze-jobb
Copy link
Contributor

introducing lf(), lt(), lr() to list local functions, types and
records respectively.
introducing fl(), ff(), tf() to remove locally defined types
and functions.
introducing save_module(FilePath) that saves local defined
functions and types to given filename.
updating fd to store the original function definition. updating rd, rf to accurately add and remove locally defined records
to local table.

@frazze-jobb frazze-jobb added the team:VM Assigned to OTP team VM label Nov 9, 2023
@frazze-jobb frazze-jobb self-assigned this Nov 9, 2023
@garazdawi
Copy link
Contributor

I'll do a review of the code later, but maybe there should be tests that the function do what they are supposed to?

@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/save_local_definition_to_module/OTP-18852 branch 2 times, most recently from 424f893 to 488ac94 Compare November 13, 2023 17:42
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

shell_default:help() needs to be updated, but otherwise this looks good to me.

lib/stdlib/src/shell.erl Show resolved Hide resolved
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/save_local_definition_to_module/OTP-18852 branch from 488ac94 to 6a334e5 Compare November 16, 2023 09:22
introducing lf(), lt(), lr() to list local functions, types and
 records respectively.
introducing fl(), ff(), tf() to remove locally defined types
 and functions.
introducing save_module(FilePath) that saves local defined
 functions and types to given filename.
updating fd to store the original function definition.
updating rd, rf to accurately add and remove locally defined records
 to local table.
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/save_local_definition_to_module/OTP-18852 branch from 6a334e5 to d6ceeb9 Compare November 16, 2023 16:28
@frazze-jobb frazze-jobb added the testing currently being tested, tag is used by OTP internal CI label Nov 22, 2023
"ok.\nok.\nok.\nok.\n{ok,my_module}.\n" = t(
<<"-type hej() :: integer().\n"
"-record(svej, {a :: hej()}).\n"
"-spec my_func(X) -> X.\n my_func(#svej{a=A}) -> A.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"-spec my_func(X) -> X.\n my_func(#svej{a=A}) -> A.\n"
"-spec my_func(X) -> X.\n"
"my_func(#svej{a=A}) -> A.\n"

for consistency

"ok.\n-record(svej,{a}).\n.\nok.\n" = t(
<<"-record(svej, {a}).\n"
"lr().">>),
"ok.\nok.\n-spec my_func(X) -> X.\nmy_func(X) ->\n X.\n\n.\nok.\n" = t(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"ok.\nok.\n-spec my_func(X) -> X.\nmy_func(X) ->\n X.\n\n.\nok.\n" = t(
"ok.\nok.\n-spec my_func(X) -> X.\nmy_func(X) ->\n X.\n\n.\nok.\n" = t(
^^^^^^^

Why are there three newlines here? Seems a bit excessive.

[Module, _] = string:split(FileName, ".", leading),
Output = (
"-module("++Module++").\n\n" ++
"-export(["++[atom_to_list(F)++"/"++integer_to_list(A)||{F,A}<-local_defined_functions(FT)]++"]).\n\n"++
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"-export(["++[atom_to_list(F)++"/"++integer_to_list(A)||{F,A}<-local_defined_functions(FT)]++"]).\n\n"++
Exports = [atom_to_list(F)++"/"++integer_to_list(A)||{F,A}<-local_defined_functions(FT)],
"-export(["++lists:join(",",Exports)++"]).\n\n"++

Need to add , inbetween exports. Also should probably be a test for multiple functions when saving module to make sure it works.

<p>Forget locally defined functions (including function specs if they exist).
</p>
</item>
<tag><c>ff({FunName,Arity})</c></tag>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected this to take two arguments, FuncName and Arity, not a two tuple.

Comment on lines 1343 to 1354
lists:join($\n,lists:map(fun(Key) ->
Spec = maps:get(Key, Output_function_specs, nospec),
Def = maps:get(Key, Output_functions, nodef),
case {Spec, Def} of
{nospec, _} -> Def;
{_, nodef} ->
{FunName, Arity} = Key,
Spec ++ "%% " ++ atom_to_list(FunName) ++ "/" ++ integer_to_list(Arity) ++ " not implemented";
{_, _} -> Spec ++ Def
end
end,
Keys)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lists:join($\n,lists:map(fun(Key) ->
Spec = maps:get(Key, Output_function_specs, nospec),
Def = maps:get(Key, Output_functions, nodef),
case {Spec, Def} of
{nospec, _} -> Def;
{_, nodef} ->
{FunName, Arity} = Key,
Spec ++ "%% " ++ atom_to_list(FunName) ++ "/" ++ integer_to_list(Arity) ++ " not implemented";
{_, _} -> Spec ++ Def
end
end,
Keys)).
string:trim(lists:join($\n,lists:map(fun(Key) ->
Spec = maps:get(Key, Output_function_specs, nospec),
Def = maps:get(Key, Output_functions, nodef),
case {Spec, Def} of
{nospec, _} -> Def;
{_, nodef} ->
{FunName, Arity} = Key,
Spec ++ "%% " ++ atom_to_list(FunName) ++ "/" ++ integer_to_list(Arity) ++ " not implemented";
{_, _} -> Spec ++ Def
end
end,
Keys))).

strip som trailing newlines

lists:join($\n,
[RecDef||{{record_def, _},RecDef} <- ets:tab2list(FT)]).
write_and_compile_module(PathToFile, Output) ->
case file:write_file(PathToFile, Output) of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case file:write_file(PathToFile, Output) of
case file:write_file(PathToFile, unicode:characters_to_binary(Output)) of

To handle unicode function names... and maybe a test?

Copy link
Contributor

github-actions bot commented Nov 29, 2023

CT Test Results

       2 files       92 suites   35m 40s ⏱️
2 004 tests 1 955 ✔️ 48 💤 1
2 308 runs  2 257 ✔️ 50 💤 1

For more details on these failures, see this check.

Results for commit a1c802f.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/save_local_definition_to_module/OTP-18852 branch from dee0a86 to a1c802f Compare November 30, 2023 09:48
@frazze-jobb frazze-jobb merged commit 42f9942 into erlang:master Dec 5, 2023
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants