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

Overhaul name_of_type #12

Merged
merged 9 commits into from
Oct 14, 2020
Merged

Overhaul name_of_type #12

merged 9 commits into from
Oct 14, 2020

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Sep 8, 2020

  • Correctly handle types that are in another name-space
  • Correctly handle Tuple{}
  • Correctly handle Symbols in typeparams (e.g. Val{:foo})
  • I think this also has some other changes, to support other things, but not sure what they are. Hopefully code-cov will tell me if I have added any features i forget about.
  • Some restructuring.

Here are tests that failed before

julia> @test_signature empty_tuple_constraint(x::Tuple{}) = 2
Test Failed at REPL[48]:5
  Expression: target[:args] == get(candidate, :args, nothing)
   Evaluated: Any[:(x::Tuple{})] == Expr[:(x::Tuple)]
ERROR: There was an error during testing

julia> @test_signature qualified_constraint(x::Base.Threads.SpinLock) = 2
Test Failed at REPL[48]:5
  Expression: target[:args] == get(candidate, :args, nothing)
   Evaluated: Any[:(x::Base.Threads.SpinLock)] == Expr[:(x::var"Base.Threads.SpinLock")]
ERROR: There was an error during testing

julia> @test_signature f_symbol_param(x::Val{:foobar}) where T = 2x
Test Failed at REPL[48]:5
  Expression: target[:args] == get(candidate, :args, nothing)
   Evaluated: Any[:(x::Val{:foobar})] == Expr[:(x::Val{foobar})]
ERROR: There was an error during testing

# it is a bug in our implementation if this error every gets hit.
isbits(x) || throw(DomainError((x, T), "not a valid type-param"))
return x
end
Copy link
Member Author

@oxinabox oxinabox Sep 8, 2020

Choose a reason for hiding this comment

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

Added these literals rather than name_of_type(x)=x so i can be sure nothing slipped through

@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #12 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   99.31%   99.42%   +0.11%     
==========================================
  Files           4        4              
  Lines         145      173      +28     
==========================================
+ Hits          144      172      +28     
  Misses          1        1              
Impacted Files Coverage Δ
src/method.jl 100.00% <100.00%> (ø)
src/function.jl 98.79% <0.00%> (+0.06%) ⬆️

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 4b175b0...cba2e15. Read the comment docs.

src/method.jl Outdated
@@ -95,7 +132,8 @@ function arguments(m::Method)
end
end

function where_parameters(x::TypeVar)
# type-vars can only show up attached to UnionAlls.
function where_parameter1(x::TypeVar)
Copy link
Member Author

Choose a reason for hiding this comment

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

This rename is because it doesn't return list of where parameters
just formats 1 of them

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we need something a bit more descriptive 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.

I can't think of a better one.
Its not unsual to have:

foos(xs) = map(foo1, xs)

This is the processing for single thing that kind of maps to the multiple things that whereparams is trying to process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not unsual to have:
foos(xs) = map(foo1, xs)

I've never seen this before (in Julia, Python or Scala that i've seen)

Copy link
Contributor

Choose a reason for hiding this comment

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

but i've no better name suggestion - sorry

Copy link
Member

@mattBrzezinski mattBrzezinski Sep 9, 2020

Choose a reason for hiding this comment

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

I've never seen this before (in Julia, Python or Scala that i've seen)

Just a different way of doing things.

julia> @time [x^2 for x in [1,2,3]]
  0.022936 seconds (41.61 k allocations: 2.128 MiB)
3-element Array{Int64,1}:
 1
 4
 9

julia> @time map(x->x^2, [1,2,3])
  0.023335 seconds (48.81 k allocations: 2.499 MiB)
3-element Array{Int64,1}:
 1
 4
 9

Copy link
Contributor

@nickrobinson251 nickrobinson251 Sep 9, 2020

Choose a reason for hiding this comment

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

I meant the 1 suffix naming convention

Copy link
Member

Choose a reason for hiding this comment

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

That makes more sense 😆

src/method.jl Outdated
@@ -95,7 +132,8 @@ function arguments(m::Method)
end
end

function where_parameters(x::TypeVar)
# type-vars can only show up attached to UnionAlls.
function where_parameter1(x::TypeVar)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we need something a bit more descriptive here.

oxinabox and others added 2 commits September 8, 2020 17:21
@omus omus changed the title Overhall name_of_type Overhaul name_of_type Sep 9, 2020
@oxinabox
Copy link
Member Author

The better name was in reprospect obvious
as long as we are ok with the idea of no constraints being considered a form of constraint

@mattBrzezinski
Copy link
Member

I'm logging off for the night, but I will take a look at this first thing tomorrow morning!

@oxinabox oxinabox merged commit ca6b948 into master Oct 14, 2020
@oxinabox oxinabox deleted the ox/overhaul branch October 14, 2020 18:15
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