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 most deprecations on Julia 0.7 #15

Merged
merged 1 commit into from
May 11, 2018
Merged

Conversation

nalimilan
Copy link
Contributor

The main change is that strings are now === even if they point to different memory areas, so we need to use pointer to check whether they are really the same. object_id is also no longer a meaningful test for strings.

I tried changing the object_id testset to use big floats instead of strings, but it triggers a crash when closing Julia. Reproducer:

@testset "ID check" begin let
    empty!(InternedStrings.pool)

    a = BigFloat(π)
    target_id = objectid(a)

    a = intern(a)
    @test objectid(a) == target_id

    b = BigFloat(π)
    @test objectid(b) != target_id
    b = intern(b)
    @test objectid(b)== target_id

    @test objectid(intern(BigFloat(π))) == target_id

#    use(a,b)
end end

@oxinabox
Copy link
Member

The BigFloat thing is:
JuliaLang/julia#26939

There is a testset waiting for them when that is fixed.

# Enable when https://github.com/JuliaLang/julia/issues/26939 is fixed
false && @testset "BigFloat" begin
let
pi1 = intern(BigFloat(π))
@test pi1 == BigFloat(π)
@test !addr_eq(pi1, BigFloat(π))
pi2 = intern(BigFloat(π))
@test addr_eq(pi1, pi2)
@testset "type inference" begin
@test pi1 isa BigFloat
@inferred intern(BigFloat(π))
end
end
end

@oxinabox
Copy link
Member

Re: the content of the PR,
I think #13 covered it all?

(It is annoying when you make a PR and someone else has made the same one, I've been bitten by that a few times.)

@nalimilan
Copy link
Contributor Author

Yeah, it's mostly redundant, but it tackles a few more deprecations. I've rebased it.

@nalimilan nalimilan changed the title Fix tests and most deprecations on Julia 0.7 Fix most deprecations on Julia 0.7 May 11, 2018
test/REQUIRE Outdated
WeakRefStrings 0.4.4
Compat 0.52
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure you mean to add this to REQUIRE not to test/REQUIRE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. 😄 Fixed.

@codecov-io
Copy link

codecov-io commented May 11, 2018

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #15   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          20     19    -1     
=====================================
- Hits           20     19    -1
Impacted Files Coverage Δ
src/InternedStrings.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecb7d92...0eed73e. Read the comment docs.

@oxinabox
Copy link
Member

Thank you, this is excellent.
Maintaining 0.7 compatibility is something I really appreciate it when someone else works out how to do..

@oxinabox oxinabox merged commit 0b9aa8e into JuliaString:master May 11, 2018
@nalimilan nalimilan deleted the nl/0.7 branch May 11, 2018 15:40
@nalimilan
Copy link
Contributor Author

Could you tag a new version? That would allow CSV to pass its tests on 0.7 after merging JuliaData/CSV.jl#204.

@oxinabox oxinabox mentioned this pull request May 11, 2018
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.

3 participants