Support (exported) macros with defoverridable/super #4848
Conversation
| @@ -32,6 +32,13 @@ name(_Module, {Name, _} = Function, Overridable) -> | |||
| {Count, _, _, _} = maps:get(Function, Overridable), | |||
| elixir_utils:atom_concat([Name, " (overridable ", Count, ")"]). | |||
|
|
|||
| %% Returns the kind (def, defp, and so on) of the given overridable function. | |||
|
|
|||
| kind(Module, FunctionOrMacro) -> | |||
There was a problem hiding this comment.
Maybe we can rename the name function to kind_and_name and return both kind and name?
There was a problem hiding this comment.
We'd need to return the FinalArgs as well (cause we want to add the '_@CALLER' arg), this is why I didn't go with kind_and_name (which is what I wanted to go with for the record 😄). Wdyt?
There was a problem hiding this comment.
We could still calculate the args later on. The benefit of kind_and_name is just to avoid doing multiple lookups both in this module and in the translator. :)
There was a problem hiding this comment.
Good point, I see what you mean now. Updated, let me know how it looks :)
|
It looks great to me, just one tiny comment left. |
| @@ -1,6 +1,6 @@ | |||
| % Holds the logic responsible for defining overridable functions and handling super. | |||
| -module(elixir_def_overridable). | |||
| -export([setup/1, overridable/1, overridable/2, name/2, super/2, store_pending/1, | |||
| -export([setup/1, overridable/1, overridable/2, name/2, kind_and_name/2, super/2, store_pending/1, | |||
There was a problem hiding this comment.
Do we still need to export name/2?
There was a problem hiding this comment.
Actually we don't, good catch! We don't need name/2 at all, I just removed it. We could go even further and make name/3 take a Count instead of the whole Overridable map because we always have that count available, but I think for now it's fine like this, name/2 is nice and scoped like this. Wdyt?
There was a problem hiding this comment.
|
@josevalim aaaand done! 👍 |
|
Ship it. |
whatyouhide
changed the title
This is a WIP PR that I'd really like to collect feedback on; it's a possible partial fix for #4830 as it fixes
super()anddefoverridablefor public macros but not private macros.Is this a good way to approach this? :)