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

[RFC] Add support for parsing 8-digit and 4-digit hex notations #371

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Dec 7, 2019

This adds the support for parsing 8-digit and 4-digit hex notations following the manner mentioned in #353 (comment).

Do not merge this PR before the decision of #353 for the consistency.

@kimikage kimikage added this to the 1.0 milestone Dec 7, 2019
@codecov
Copy link

codecov bot commented Dec 7, 2019

Codecov Report

Merging #371 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #371     +/-   ##
=========================================
+ Coverage   77.94%   78.04%   +0.1%     
=========================================
  Files          11       11             
  Lines         866      870      +4     
=========================================
+ Hits          675      679      +4     
  Misses        191      191
Impacted Files Coverage Δ
src/parse.jl 97.01% <100%> (+0.18%) ⬆️

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 47e3750...e5f6923. Read the comment docs.

@kimikage
Copy link
Collaborator Author

kimikage commented Dec 7, 2019

Please note the following:

julia> parse(Colorant, "0xFF8800AA") # opaque purple
ARGB{N0f8}(0.533,0.0,0.667,1.0)

julia> parse(RGBA, "0xFF8800AA") # still opaque purple
RGBA{N0f8}(0.533,0.0,0.667,1.0)

We have another option, which refers to the first argument of parse(). However, I don't think it is good to depend on the context which doesn't appear in the string.

@kimikage kimikage mentioned this pull request Dec 8, 2019
4 tasks
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This is really nice. Do all those @inbounds annotations really make a difference?

@kimikage
Copy link
Collaborator Author

Perhaps there is little difference in terms of the speed on Julia v1.3.0 and v1.0.5. They are slower than v1.2.0 (especially on Windows) for some unclear reason.

I added @inbounds because the boundary checks are very disturbing when trying to clarify the cause of stall or the issue JuliaLang/julia#34055, rather than because of the speed. (In other words, the native codes are quite different.)

Since I don't have enough time to investigate and the cause has not been identified, @inbounds has actually little benefit for the end users for now.

@kimikage
Copy link
Collaborator Author

I merged #390. I also changed != nothing to !== nothing and removed some @inbounds.

@kimikage
Copy link
Collaborator Author

kimikage commented Jan 6, 2020

Since PR #387 was merged, we can merge this.

@kimikage kimikage merged commit f1047da into JuliaGraphics:master Jan 6, 2020
@kimikage kimikage deleted the parse_hex8 branch January 6, 2020 03:17
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