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.

Sign up for GitHub

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

Jump to bottom

[OTP-21] Remove get_stacktrace calls to be compatible with OTP21 #10

Closed
wants to merge 1 commit into from
Closed

[OTP-21] Remove get_stacktrace calls to be compatible with OTP21 #10

wants to merge 1 commit into from

Conversation

maximvl
Copy link

@maximvl maximvl commented Dec 14, 2018

Hey Carlos, here's the change we discussed: #9

@maximvl
Copy link
Author

maximvl commented Dec 17, 2018

I can see Dialyzer check failed, not sure why:

%%% xdb_meta_SUITE ==> {{badmatch,["xdb_mnesia_adapter.erl:92: Function rollback/2 has no local return\n",
            "xdb_test_mnesia_repo.erl:266: Function rollback/1 has no local return\n",
            "xdb_lib_SUITE.erl:136: The created fun has no local return\n",
            "xdb_lib_SUITE.erl:140: The created fun has no local return\n",
            "xdb_lib_SUITE.erl:144: The created fun has no local return\n"]},
 [{ktn_meta_SUITE,dialyzer,1,
                  [{file,"/home/travis/build/cabol/cross_db/_build/test/lib/katana_test/src/ktn_meta_SUITE.erl"},
                   {line,73}]},
  {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1546}]},
  {test_server,run_test_case_eval1,6,[{file,"test_server.erl"},{line,1062}]},
  {test_server,run_test_case_eval,9,[{file,"test_server.erl"},{line,994}]}]}

cabol
cabol reviewed Dec 20, 2018
View reviewed changes
src/xdb_lib.erl
@@ -144,19 +144,19 @@ reduce_while(Fun, AccIn, List) when is_function(Fun, 2) ->
catch
throw:{halt, AccOut} ->
AccOut;
Kind:Reason ->
erlang:raise(Kind, Reason, erlang:get_stacktrace())
_:Reason ->
Copy link
Owner

@cabol cabol Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Let's remove this clause, we don't need it, if it is not throw :... by default an error is raised.

Copy link
Owner

Choose a reason for hiding this comment

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

@maximvl did you have the chance to address the suggested fix above ^^ ??

Copy link
Author

Choose a reason for hiding this comment

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

@cabol sorry for long reply, could you tell which exact part we can remove here? whole catch?

Copy link
Owner

Choose a reason for hiding this comment

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

I meant you should remove this pattern-match _:Reason

Copy link
Owner

Choose a reason for hiding this comment

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

@maximvl what about this? For me it should be removed, this pattern match doesn't make sense since by default the original error is raised.

@cabol
Copy link
Owner

cabol commented Dec 20, 2018

Yes, no idea what dialyzer is complaining of, we should track where might be a spec violation maybe. BTW, if you run dialyzer locally (rebar3 dialyzer) what happen?

@maximvl
Copy link
Author

maximvl commented Apr 1, 2019 •
edited
Loading

@cabol locally dialyzer checks pass on this branch using Erlang 21

@maximvl maximvl force-pushed the otp21-migration branch 2 times, most recently from 11ccd17 to eaf2d3c Compare July 2, 2019 16:13
maximvl
maximvl commented Jul 2, 2019
View reviewed changes
src/test/xdb_repo_basic_test.erl
@@ -45,7 +45,12 @@
init_per_testcase(_, Config) ->
Repo = xdb_lib:keyfetch(repo, Config),
{ok, _} = Repo:start_link(),
{_, _} = Repo:delete_all(person),
%% On the first run this table does not exist and this will throw an error
Copy link
Author

Choose a reason for hiding this comment

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

I had to add this, otherwise, it always fails CI

@maximvl
Copy link
Author

maximvl commented Jul 2, 2019

@cabol Hi, I updated the PR but not sure how to trigger CI, could you check?

maximvl
maximvl commented Jul 2, 2019
View reviewed changes
src/xdb_transform.erl Outdated
@@ -268,6 +271,14 @@ build_repo_fun(Repo, Adapter, Mod, Fun, Args) ->
Body = build_repo_fun_body(Fun, "~p, ~p", Args),
?Q(text(Body, [Mod, Repo, Adapter])).

%%@private
build_repo_fun_spec(_, rollback, _) ->
?Q(lists:flatten(["-spec ", "rollback", "(any()) -> no_return()."]));
Copy link
Author

Choose a reason for hiding this comment

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

This spec is required because new Dialyzer is very strict for no_return() functions

@maximvl
Copy link
Author

maximvl commented Jul 2, 2019

I added a spec generation to xdb_transform because new Dialyzer is very strict and requires no_return() spec explicitly, in our case for the rollback function

@maximvl maximvl force-pushed the otp21-migration branch 2 times, most recently from 424a59c to 6a23a29 Compare July 3, 2019 10:40
maximvl
maximvl commented Jul 3, 2019
View reviewed changes
test/xdb_lib_SUITE.erl
@@ -1,5 +1,7 @@
-module(xdb_lib_SUITE).

-dialyzer({nowarn_function, t_raise/1}).
Copy link
Author

@maximvl maximvl Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't find any other way to stop dialyzer complaining about funs being no_return() inside t_raise

@maximvl
Copy link
Author

maximvl commented Jul 3, 2019

I'm going to replace this PR with a fresh one

@maximvl maximvl closed this Jul 3, 2019
@maximvl maximvl mentioned this pull request Jul 3, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cabol cabol cabol left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maximvl @cabol