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

Fix ~370 invalidations from Expr(:ncat, ...) pretty-printing #41877

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Aug 13, 2021

These get invalidated by loading Static.jl, specifically the method

Base.convert(::Type{T}, ::StaticInt{N}) where {T<:Number,N} = convert(T, N)

CC @ChrisRackauckas

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

LGTM. Should we backport this to 1.7 as well? Seems pretty harmless to me.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 16, 2021

A codegen performance test seems to be unexpectedly unhappy with this, but LGTM

@vtjnash vtjnash closed this Aug 16, 2021
@vtjnash vtjnash reopened this Aug 16, 2021
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 17, 2021

How can I see the codegen performance test?

I could get rid of the Int(x)::Int and just make it x::Int. All entries here come from the parser, right @simeonschaub? So we don't have to worry about someone putting a different integer type in the expression?

@simeonschaub
Copy link
Member

Yes, that might be preferable. A type assertion error seems more appropriate here than a conversion error in case someone generated an invalid AST.

These get invalidated by loading Static.jl, specifically the method
```
Base.convert(::Type{T}, ::StaticInt{N}) where {T<:Number,N} = convert(T, N)
```
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Aug 18, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 18, 2021

It was probably just something transient and weird about CI at the time

@timholy timholy merged commit 232ee11 into master Aug 18, 2021
@timholy timholy deleted the teh/inval_show_ncat branch August 18, 2021 08:19
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Aug 25, 2021
KristofferC pushed a commit that referenced this pull request Aug 25, 2021
These get invalidated by loading Static.jl, specifically the method
```
Base.convert(::Type{T}, ::StaticInt{N}) where {T<:Number,N} = convert(T, N)
```

(cherry picked from commit 232ee11)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…ng#41877)

These get invalidated by loading Static.jl, specifically the method
```
Base.convert(::Type{T}, ::StaticInt{N}) where {T<:Number,N} = convert(T, N)
```
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…ng#41877)

These get invalidated by loading Static.jl, specifically the method
```
Base.convert(::Type{T}, ::StaticInt{N}) where {T<:Number,N} = convert(T, N)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants