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

Change LineNumber nodes to point at definition site #311

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Mar 24, 2021

Before:

julia> @which rrule(typeof, 1.0)
rrule(::typeof(typeof), var"269"; kwargs...) in ChainRules at [...]/rule_definition_tools.jl:352

After:

julia> @which rrule(typeof, 1.0)
rrule(::typeof(typeof), var"269"; typeof_pullback) in ChainRules at [...]/src/rulesets/Core/core.jl:8

N.B.:, currently includes the commit from #308, because it touches the same code, but should be automatically rebased once that is in.

@oxinabox
Copy link
Member

Yesss, thanks, i have been meeting to do this for ages.
Ever since #58
but working out how for multiexpression functions proved annoying.

It would be good to add this to the Julia docs?

@@ -1,4 +1,13 @@
# These are some macros (and supporting functions) to make it easier to define rules.
using Base.Meta

macro strip_linenos(expr)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It operates on the quote itself, rather than the interpolated expression.

Copy link
Member

Choose a reason for hiding this comment

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

I see, can we add a comment to this effect? or a docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, feel free to add whatever you think would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Is the following correct?

Suggested change
macro strip_linenos(expr)
"This should be put before a `quote`, to stop the quote from having line numbers recorded"
macro strip_linenos(expr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that is how to use it, but it's a bit of an odd description of the macro. It just deleted linenumber nodes in whatever block you put it in front of. E.g. if you did:

@strip_linenos function foo(x)
<blah>
end

then that function wouldn't have line numbers either.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Needs rebasing and bumping the verison number then we can merge.

Before:
```
julia> @which rrule(typeof, 1.0)
rrule(::typeof(typeof), var"269"; kwargs...) in ChainRules at [...]/rule_definition_tools.jl:352
```

After:
```
julia> @which rrule(typeof, 1.0)
rrule(::typeof(typeof), var"269"; typeof_pullback) in ChainRules at [...]/src/rulesets/Core/core.jl:8
```
@Keno Keno force-pushed the kf/ruledeflineno branch from bb1cef3 to 227c5d7 Compare March 25, 2021 17:41
@oxinabox
Copy link
Member

BlockDiagonals failure is unrelated.

@oxinabox
Copy link
Member

oxinabox commented Mar 25, 2021

ChainRules.jl failures are unrelated

@oxinabox oxinabox merged commit bb81421 into JuliaDiff:master Mar 25, 2021
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