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

Add edit, less and code_* macros, mirroring which #5832

Merged
merged 1 commit into from
Apr 8, 2014

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Feb 17, 2014

Also add methods for edit and less that take ::Any vararg (again, mirroring which), and specify that the previous definitions take a tuple of ::(DataTypes...).

This was remarkably easy with the recent refactoring of which. The number of changed lines is deceiving due to the indentation of a for loop (over :which,:edit,:less, and now additionally :code_typed,:code_lowered,:code_llvm,:code_native) and an @eval block around the existing @which definition. The only real changes required were to change :which to $fname and adding the equivalent method signatures. Pretty cool that it just works.

I've since modified to macro to determine the types of the arguments itself. This prevents a potential ambiguity between an argument that happens to be a tuple of types and specifying the types of the arguments. But to make this work for all of the above functions, which(f,args…) needs to be deprecated in favor of which(f,types::(Type…)) (or the macro itself). Both can co-exist, but there's that very small corner case of the ambiguity in discerning which which the user is asking for.

@ivarne
Copy link
Member

ivarne commented Feb 17, 2014

Does the same approach work for @code_typed, @code_lowered, @code_llvm and @code_native also?

Crosslinking: #5674 and #5779

@ivarne
Copy link
Member

ivarne commented Feb 17, 2014

Also, this needs to be mentioned in NEWS.md and in the documentation docs/stdlib, before it can be merged.

@mbauman
Copy link
Member Author

mbauman commented Feb 17, 2014

Oh, cool, yes, I think it should work with all the code_* methods, too. And edit and less can seamlessly extend to all Callables, too… which is something else that had bugged me for a while now.

Question: In the code_* methods, they all take a Tuple of ::(Any...,). Since I'm adding methods that take varargs of the objects themselves, I had made the Tuple more specific (::(DataType...,)) so as to reduce the chances of the two methods getting confused. Are there cases where a tuple of types will contain anything other than DataType? I should probably change them to be consistent… which way should I go? Other than that, I think this could be ready to go.

@carlobaldassi
Copy link
Member

Are there cases where a tuple of types will contain anything other than DataType?

UnionType comes to mind.

@mbauman
Copy link
Member Author

mbauman commented Feb 17, 2014

Ah, sure. The abstract Type is better (subtypes : {DataType,TypeConstructor,UnionType}).

But is that enough? What is ANY? TypeVars? They don't seem to be documented and aren't implemented in Julia.

@quinnj
Copy link
Member

quinnj commented Feb 17, 2014

I just haven't had time to really work on #5779, so this is great!

@vtjnash
Copy link
Member

vtjnash commented Feb 18, 2014

these are things related to defining functions, that disappear when running the code.

ANY is equivalent to Any, but it prevents code specialization on that argument

TypeVar is the Julia type of the T in function f{T}(x::T) (before compilation, when it will be replaced with the runtime type of x)

@mbauman
Copy link
Member Author

mbauman commented Feb 18, 2014

Thanks for the explanation, @vtjnash. So that's not a concern. But there's still a corner case here: tuples. which(start,(1,2)) will send Julia into an infinite method resolution loop. Bah.

@simonbyrne
Copy link
Contributor

Unfortunately I don't think its going to be possible to define one function that does both: for instance which(foo,(Type1,Type2)) could refer to two possible signatures: foo(a::Type1,b::Type2) or foo(::(Type{Type1},Type{Type2})) (granted, this signature is not so common, but possible).

The easiest way around this would be to define different functions: one for arguments and one for types, unfortunately this would mean breaking the existing behaviour of either which, or less and friends.

@mbauman
Copy link
Member Author

mbauman commented Feb 24, 2014

Well, this is stretching my macro tinkering abilities to the limit, but I think I've managed to get the type conversion to occur within the macro. Of course, as Simon mentions, that now makes this simple macro tinkering a breaking change.

This ended up being more than I intended to bite off. But hopefully it's helpful, if only for the idea of sharing the code from this macro.

@ivarne
Copy link
Member

ivarne commented Mar 26, 2014

Bump!
What is holding up this PR? I miss the @less macro so much when I try to trace trough code to find errors.

@mbauman
Copy link
Member Author

mbauman commented Mar 26, 2014

I think I'm good now. I rebased and added one minor extra commit to deprecate the which(f,args...) syntax in favor of the macro. If both are defined there's a potential confusion between which one the user wants — is the argument a tuple itself or is it the types of the arguments.

All works well in my tests. It just needs a review.

@ivarne
Copy link
Member

ivarne commented Apr 4, 2014

What keeps these nice macros from being merged? They are just convenience for the REPL, and will probably never break code if we find a reason to remove them at a later stage.

@StefanKarpinski @JeffBezanson

@mbauman
Copy link
Member Author

mbauman commented Apr 4, 2014

Well, there was a big tree conflict with dd60925, but I've pseudo-rebased/squashed. So it's good to go again.

@Keno
Copy link
Member

Keno commented Apr 4, 2014

+1. LGTM, but I'll let @JeffBezanson have a look and do the merge.

@StefanKarpinski
Copy link
Member

I guess it just gives me pause to export so many new names. It's @JeffBezanson's call.

typesof(args...) = map(typeof,args)

for fname in [:which,:less,:edit,:code_typed,:code_lowered,:code_llvm,:code_native]
@eval begin
Copy link
Member

Choose a reason for hiding this comment

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

This @eval loop should be removed. There is no reason to copy this entire function body when it is basically identical for each case. Instead, there should be a single code-generating function that takes fname as an argument, and each macro can be a trivial wrapper around this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes. Something about having a macro-generating macro struck me as not right, but a better way didn't occur to me off the top of my head. That's much simpler.

The macros are programmatically generated, based on the previous macro for which.  The arguments are evaluated and their types are determined so the corresponding function may be called.

Adds news and documentation.
@mbauman
Copy link
Member Author

mbauman commented Apr 4, 2014

Thanks for the comments! I think I've addressed them in the updated patch.

@JeffBezanson - I'm not entirely sure I understood your comment. I've broken the code generation out into a separate function, but I don't understand how I'd remove the @eval macro.

@@ -450,3 +450,5 @@ end
export nnz

scale!{T<:Base.LinAlg.BlasReal}(X::Array{T}, s::Complex) = error("scale!: Cannot scale a real array by a complex value in-place. Use scale(X::Array{Real}, s::Complex) instead.")

@deprecate which(f::Callable, args...) @which f(args...)
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 this should be deprecated. I'd still want to use the function form in various cases. And doesn't the macro generate calls to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the macro generates calls to which(f,t::(Type…)). What I'm deprecating here is the previous definition of which, which determines the types of its arguments itself.

Both could stay in base, but if the user called a function which(f,(Int,)) there's an ambiguity between whether the user was looking for the method that would be called with f((Int,)) or f(1). It's a really slight corner-case, though, and it fails gracefully to the more common case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, in the current definitions for all the methods (which, edit, less, code_...), which is the odd man out. All the others get the arguments as tuple of types. So for this macro to generically call all of them I needed to change which's signature.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you are right.

@ivarne
Copy link
Member

ivarne commented Apr 4, 2014

@StefanKarpinski these names are not really new. It's just macro versions of functions that have a clumsy interface, that makes them hard to use for quick debugging.

Edit: It will increase length(names(Base)) though, but that issue should rather be fixed by making parts of base optional to load/ moved into packages.

@StefanKarpinski
Copy link
Member

Yeah, true enough. We should do it.

@mbauman
Copy link
Member Author

mbauman commented Apr 5, 2014

Well, I can fix the error messages simply enough, but this is now blocked by #6426.

@JeffBezanson
Copy link
Member

Unblocked!

StefanKarpinski added a commit that referenced this pull request Apr 8, 2014
Add edit, less and code_* macros, mirroring which
@StefanKarpinski StefanKarpinski merged commit 648f74e into JuliaLang:master Apr 8, 2014
@mbauman mbauman deleted the editmacro branch April 8, 2014 23:30
@mbauman
Copy link
Member Author

mbauman commented Apr 8, 2014

Woohoo! Thanks guys!

@StefanKarpinski
Copy link
Member

Thanks for being patient while that simmered.

@StefanKarpinski
Copy link
Member

And for doing the work in the first place, of course.

@quinnj
Copy link
Member

quinnj commented Apr 8, 2014

Awesome. Looking forward to this.

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.

9 participants