Skip to content

Conversation

jonatanklosko
Copy link
Member

Implements suggestions from erlang/otp#8945.

Comment on lines -44 to +49
get_ann([{generated, true} | T], _, Line) -> get_ann(T, true, Line);
get_ann([{line, Line} | T], Gen, _) when is_integer(Line) -> get_ann(T, Gen, Line);
get_ann([_ | T], Gen, Line) -> get_ann(T, Gen, Line);
get_ann([], Gen, Line) -> erl_anno:set_generated(Gen, erl_anno:new(Line)).
get_ann([{generated, true} | T], _, Line, Column) -> get_ann(T, true, Line, Column);
get_ann([{line, Line} | T], Gen, _, Column) when is_integer(Line) -> get_ann(T, Gen, Line, Column);
get_ann([{column, Column} | T], Gen, Line, _) when is_integer(Column) -> get_ann(T, Gen, Line, Column);
get_ann([_ | T], Gen, Line, Column) -> get_ann(T, Gen, Line, Column);
get_ann([], Gen, Line, undefined) -> erl_anno:set_generated(Gen, erl_anno:new(Line));
get_ann([], Gen, Line, Column) -> erl_anno:set_generated(Gen, erl_anno:new({Line, Column})).
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously this ignored column from meta, that's why the assertions in warning_test.exs now include a column as well :)


UniqFun = fun({Def, _Meta}) -> Def end,

lists:uniq(UniqFun, lists:sort(LineComparator, Entries)).
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we actually need uniqueness here. We only need sorting but lists:usort is faster than lists:sort, which is why we called usort before. I don't think we actually expect duplicate tuples in there!

Copy link
Member Author

Choose a reason for hiding this comment

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

There can be duplicates for callbacks with multiple clauses. Looks like that's not the case for definitions though!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@josevalim
Copy link
Member

josevalim commented Oct 18, 2024

Just some small nitpicks. Once the Erlang/OTP PRs have been merged (erlang/otp#8945), I will merge this one. :)

@josevalim
Copy link
Member

I will merge this, since the Erlang/OTP PR for the Docs reference has been merged, although we are waiting on updates on how we should deal with ranges, based on erlang/otp#8966 and erlang/otp#8975.

@josevalim josevalim merged commit 1c19c60 into elixir-lang:main Oct 30, 2024
4 of 9 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants