Skip to content

supports @inline/@noinline annotations within a function body#41312

Merged
aviatesk merged 4 commits intomasterfrom
avi/doinline
Jun 23, 2021
Merged

supports @inline/@noinline annotations within a function body#41312
aviatesk merged 4 commits intomasterfrom
avi/doinline

Conversation

@aviatesk
Copy link
Copy Markdown
Member

Separated from #40754 in order to make the review/discussion more easier and clearer.

The primary motivation for this change is to annotate
@inline/@noinline to anonymous functions created from do block:

f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end

We can extend the grammar so that we have special "declaration-macro"
supports for do-block functions like:

f() @inline do # makes this anonymous function to be inlined
    ... # function body
end

but I'm not sure which one is better.

Following the earlier discussion,
this commit implements the easiest solution.


/cc @dghosef , @tkf

Separated from #40754 for the sake of easier review.

The primary motivation for this change is to annotate
`@inline`/`@noinline` to anonymous functions created from `do` block:
```julia
f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end
```

We can extend the grammar so that we have special "declaration-macro"
supports for `do`-block functions like:
```julia
f() @inline do # makes this anonymous function to be inlined
    ... # function body
end
```
but I'm not sure which one is better.

Following [the earlier discussion](#40754 (comment)),
this commit implements the easiest solution.

Co-authored-by: Joseph Tan <jdtan638@gmail.com>
Comment thread base/expr.jl
macro noinline(ex)
esc(isa(ex, Expr) ? pushmeta!(ex, :noinline) : ex)
end
macro noinline() Expr(:meta, :noinline) end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean we can delete Core.@_noinline_meta() also?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we can unify them. Will do that in a follow up PR though.

@JeffBezanson
Copy link
Copy Markdown
Member

f() @inline do

❌ Do not want! 😄

Comment thread base/expr.jl Outdated
Comment thread base/expr.jl Outdated
Comment thread NEWS.md Outdated
@aviatesk aviatesk merged commit 37c0b06 into master Jun 23, 2021
@aviatesk aviatesk deleted the avi/doinline branch June 23, 2021 00:30
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…liaLang#41312)

* supports `@inline`/`@noinline` annotations within a function body

Separated from JuliaLang#40754 for the sake of easier review.

The primary motivation for this change is to annotate
`@inline`/`@noinline` to anonymous functions created from `do` block:
```julia
f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end
```

We can extend the grammar so that we have special "declaration-macro"
supports for `do`-block functions like:
```julia
f() @inline do # makes this anonymous function to be inlined
    ... # function body
end
```
but I'm not sure which one is better.

Following [the earlier discussion](JuliaLang#40754 (comment)),
this commit implements the easiest solution.

Co-authored-by: Joseph Tan <jdtan638@gmail.com>

* Update base/expr.jl

* Update base/expr.jl

* Update NEWS.md

Co-authored-by: Joseph Tan <jdtan638@gmail.com>
@timholy
Copy link
Copy Markdown
Member

timholy commented Sep 4, 2021

I've created the shiny new "Add to Compat" label. Keep in mind that's not intended to be a requirement for the PR author before merging, and it can be done by anyone, anytime. The intent is that by tagging PRs with this label, we may have a better sense for likely trouble when we start running release-validation tests, and it may also encourage dissemination of excellent new features across the package ecosystem.

While this looks eminently backportable via Compat.jl, the intent is that this can be used even when the only possible version that could be added to Compat.jl is a "stub" implementation that simply avoids an error, allowing the new construct to be used in packages even for Julia versions where it won't actually have any effect.

Motivated by #42087.

@aviatesk
Copy link
Copy Markdown
Member Author

aviatesk commented Sep 4, 2021

Sounds great, thanks.

Do you think we want to tag #41328 with the label also ?
Probably we may want to add the stub implementations of the callsite annotations to Compat.jl as well so that packages can keep being compatible with older versions, but they're just new features and won't lead to release-validation tests.

@timholy
Copy link
Copy Markdown
Member

timholy commented Sep 4, 2021

Added to #41328 and #41931.

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.

4 participants