Skip to content

some refactoring of the overload macro #38

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

Closed
wants to merge 1 commit into from
Closed

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Feb 28, 2020

  • call it @vml_overload instead of just @overload since it is exported and @overload is a very generic name
  • no longer overload on Array but only on Vector. Calling it on Array meant it included matrices and gave different semantics for e.g. exp(M::Matrix). That can break existing code and is just not acceptable.
  • Constraint the element types for those that are applicable to the function, so e.g. exp(::Vector{BigInt}) will no longer try to call VML.
  • Require one to give the module for what to overload in, e.g. @vml_overload Base.exp for the same reason one has to write Base.exp when overloading something locally. This means that there is no need to depend on SpecialFunctions anymore.

@KristofferC
Copy link
Member Author

Just as a personal note, I am not sure this type of macro is a good idea. This is doing "type piracy" and if someone does this in a library it will add this behavior everywhere in the program. New users might get confused when cos(::Array) actually gives an answer when the documentation says it is not defined etc. It seems clearer to me to just write IntelVectorMath.cos.

@Crown421
Copy link
Collaborator

Thank you very much, this PR together with adding in the artifact (which is absolutely wonderful) removes almost all external dependencies.

Regarding the macro: I personally use the exported alias (IVM) or a self defined one (for IVM.cos) and not the macro, but there seemed to be some demand for it.
I remember some thread in the discourse that concluded with saying that imported function in a library/ package should be always qualified for clarity.

@KristofferC KristofferC mentioned this pull request Feb 29, 2020
@Crown421
Copy link
Collaborator

Crown421 commented Mar 1, 2020

Closed in favour of #42

@Crown421 Crown421 closed this Mar 1, 2020
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