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.

Sign up for GitHub

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

Jump to bottom

Support more forms of at-nospecialize. #409

Merged
merged 2 commits into from
Nov 9, 2017
Merged

Support more forms of at-nospecialize. #409

merged 2 commits into from
Nov 9, 2017

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Nov 7, 2017

Suggestions for tests?

yuyichao
yuyichao requested changes Nov 7, 2017
View reviewed changes
src/Compat.jl Outdated
elseif isa(arg, Expr) && arg.head == :(::)
# @nospecialize(arg::typ)
arg, typ = arg.args
return :($(esc(arg))::ANY)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, the two forms are not equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not equivalent, in that the arg::typ form adds a type assert, or how does it differ?

Copy link
Contributor

@yuyichao yuyichao Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

They have different dispatch behavior. You are basically turning a arg::typ to arg::Any for dispatch purpose and that's not OK. There's no way before 0.7 to specify a non-Any dispatch argument to not be specialized on so it's impossible to do what you want to do here.

For user of this, if arg::Any dispatch works for you, you should just use @nospecialize(arg) instead of @nospecialize(arg::typ) so that there won't actually be any specialization. If arg::Any dispatch doesn't work for you, than what you are doing here is absolutely wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, pretty obvious in retrospect. I'll just return the expression without modifications then (like you were doing).

@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2017 •
edited
Loading

Suggestions for tests?

Make sure dispatch and kw type contraints still works. (i.e. don't remove the old one)

yuyichao
yuyichao reviewed Nov 7, 2017
View reviewed changes
test/runtests.jl Outdated
no_specialize(@nospecialize(x)) = sin(1)
no_specialize(@nospecialize(x::Integer)) = sin(2)
@test no_specialize(1.0) == sin(1)
@test no_specialize(1) == sin(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

And you are defeating the whole point of the test. The test is added specifically to prevent making changes like this one.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2017

Note that the new test should pass with no problem with the old definition. What you can add "better" support for is @nospecialize(a=b)

@maleadt
Copy link
Member Author

maleadt commented Nov 7, 2017

Yes, I missed the subtlety with the arg::typ form (although a comment could have been useful).
Will fix and update.

@maleadt
Copy link
Member Author

maleadt commented Nov 8, 2017

OK, should be good to go. @yuyichao?

yuyichao
yuyichao reviewed Nov 8, 2017
View reviewed changes
test/runtests.jl Outdated
@@ -763,6 +763,11 @@ no_specialize(@nospecialize(x)) = sin(1)
no_specialize(@nospecialize(x::Integer)) = sin(2)
@test no_specialize(1.0) == sin(1)
@test no_specialize(1) == sin(2)
no_specialize_kw(@nospecialize(x=0)) = sin(1)
no_specialize_kw(@nospecialize(x::Integer=0)) = sin(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a method overwrite here...

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've split them up.

yuyichao
yuyichao approved these changes Nov 8, 2017
View reviewed changes
Copy link
Contributor

@yuyichao yuyichao left a comment

Choose a reason for hiding this comment

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

lgtm

@maleadt
Copy link
Member Author

maleadt commented Nov 8, 2017

OK, will merge tomorrow. Can I tag a new version after that? For use in CUDAnative.

@maleadt maleadt merged commit fbd9cb2 into master Nov 9, 2017
@maleadt maleadt deleted the tb/nospecialize branch November 9, 2017 07:42
martinholters added a commit that referenced this pull request Sep 29, 2019
@martinholters
Drop compat code for at-nospecialize (from #385 and #409)
306bf6e
martinholters added a commit that referenced this pull request Sep 29, 2019
@martinholters
Drop compat code for at-nospecialize (from #385 and #409)
2dbd40b
martinholters added a commit that referenced this pull request Oct 8, 2019
@martinholters
Drop compat code for at-nospecialize (from #385 and #409)
0ba707d
martinholters added a commit that referenced this pull request Oct 13, 2019
@martinholters
Drop support for Julia versions <1.0 (#665)
4fa5519 
* Bump required Julia version to 1.0

* Remove compatibility support code for:
  * `at-__MODULE__` (from #363)
  * `devnull`, `stdin`, `stdout`, and `stderr` from #499
  * `at-nospecialize` (from #385 and #409)
  * `isabstracttype` and `isconcretetype` (from #477)
  * `invokelatest` from #424
  * array-like access to `Cmd` from #379
  * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399
  * `logdet(::Any)` fallback from #382
  * `chol(::UniformScaling)` from #382
  * `pushfirst!`, `popfirst!` from #444
  * `fieldcount` from #386
  * `read(obj, ::Type{String})` from #385 and #580
  * `InexactError`, `DomainError`, and `OverflowError` constructors from #393
  * `corrected` kw arg to `cov` from #401
  * `adjoint` from #401
  * `partialsort` from #401
  * `pairs` from #428
  * `AbstractRange` from #400
  * `rtoldefault` from #401
  * `Dates.Period` rounding from #462
  * `IterativeEigensolvers` from #435
  * `occursin` from #520
  * `Char` concatenation from #406
  * `BitSet` from #407
  * `diagm` and `spdiagm` with pairs from #408
  * `Array` c'tors from `UniformScaling` from #412 and #438
  * `IOContext` ctor taking pairs from #427
  * `undef` from #417 and #514
  * `get` on `ENV` from #430
  * `ComplexF...` from #431
  * `tr` from #614
  * `textwidth` from #644
  * `isnumeric` from #543
  * `AbstractDict` from #435
  * `axes` #435 and #442
  * `Nothing` and `Cvoid` from #435
  * `Compat.SuiteSparse` from #435
  * `invpermute!` from #445
  * `replace` with a pair from #445
  * `copyto!` from #448
  * `contains` from #452
  * `CartesianIndices` and `LinearIndices` from #446, #455, and #524
  * `findall` from #466 (and #467).
  * `argmin` and `argmax` from #470, #472, and #622
  * `parentmodule` from #461
  * `codeunits` from #474
  * `nameof` from #471
  * `GC` from #477
  * `AbstractDisplay` from #482
  * `bytesavailable` from #483
  * `firstindex` and `lastindex` from #480 and #494
  * `printstyled` from #481
  * `hasmethod` from #486
  * `objectid` from #486
  * `Compat.find*` from #484 and #513
  * `repr` and `showable` from #497
  * `Compat.names` from #493 and #505
  * `Compat.round` and friends #500, #530, and #537
  * `IOBuffer` from #501 and #504
  * `range` with kw args and `LinRange` from #511
  * `cp` and `mv` from #512
  * `indexin` from #515
  * `isuppercase` and friends from #516
  * `dims` and `init` kwargs from #518, #528, #590, #592, and #613
  * `selectdim` from #522 and #531
  * `repeat` from #625
  * `fetch(::Task)` from #549
  * `isletter` from #542
  * `isbitstype` from #560
  * `at-cfunction` from #553 and #566
  * `codeunit` and `thisind` and friends from #573
  * `something` from #562
  * `permutedims` from #582
  * `atan` from #574
  * `split` and `rsplit` from #572
  * `mapslices` from #588
  * `floatmin` and `floatmax` from #607
  * `dropdims` from #618
  * required keyword arguments from #586
  * `CartesianRange` in `at-compat` from #377
  * `finalizer` from #416
  * `readline`, `eachline`, and `readuntil` from #477, #541, and #575
  * curried `isequal`, `==`, and `in` from #517
  * `Some` from #435 and #563
  * `at-warn` and friends from #458

* Remove old deprecations

* Deprecate:
  * `Compat.Sockets` from #545 and #594
  * `TypeUtils` from #304
  * `macros_have_sourceloc` from #355
  * `Compat.Sys` from #380, #433, and #552
  * `Compat.MathConstants` from #401
  * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404
  * `Compat.Dates` from #413
  * `Compat.Libdl` from #465 (and #467)
  * `AbstractDateTime` from #443
  * `Compat.Printf` from #435
  * `Compat.LinearAlgebra` from #463
  * `Compat.SparseArrays` from #459
  * `Compat.Random` from #460, #601, and #647
  * `Compat.Markdown` from #492
  * `Compat.REPL` from #469
  * `Compat.Serialization` from #473
  * `Compat.Statistics` from #583
  * `Fix2` from #517
  * `Compat.Base64` from #418
  * `Compat.Unicode` from #432 and #507
  * `notnothing` from #435 and #563
  * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451
  * `enable_debug(::Bool)` from #458
  * `Compat.Distributed` from #477
  * `Compat.Pkg` from #485
  * `Compat.InteractiveUtils` from #485
  * `Compat.LibGit2` from #487
  * `Compat.UUIDs` from #490
  * `Compat.qr` from #534
  * `Compat.rmul!` from #546
  * `Compat.norm` abd friends from #577

* Remove obsolete README entry, missed in #385

* Remove obsolete tests (e.g. missed in #372)

* Remove obsolete `VERSION` conditionals and some minor clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuyichao yuyichao yuyichao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maleadt @yuyichao