Add missing type restrictions to benchmark directory #15688
Conversation
src/benchmark/bm.cr
Outdated
| @@ -44,13 +44,13 @@ module Benchmark | |||
| end | |||
|
|
|||
| # Reports a single benchmark unit. | |||
| def report(label = " ", &block) | |||
| def report(label : String = " ", &block) : Nil | |||
There was a problem hiding this comment.
This was actually leaking @reports below, I manually set this to Nil as I don't think that was intended.
src/benchmark/bm.cr
Outdated
| @@ -44,13 +44,13 @@ module Benchmark | |||
| end | |||
|
|
|||
| # Reports a single benchmark unit. | |||
| def report(label = " ", &block) | |||
| def report(label : String = " ", &block) : Nil | |||
There was a problem hiding this comment.
suggestion: We could also add a restriction to the block parameter.
| def report(label : String = " ", &block) : Nil | |
| def report(label : String = " ", &block : ->) : Nil |
src/benchmark/ips.cr
Outdated
| @@ -31,7 +31,7 @@ module Benchmark | |||
| end | |||
|
|
|||
| # Adds code to be benchmarked | |||
| def report(label = "", &action) : Benchmark::IPS::Entry | |||
| def report(label : String = "", &action) : Benchmark::IPS::Entry | |||
There was a problem hiding this comment.
ditto:
| def report(label : String = "", &action) : Benchmark::IPS::Entry | |
| def report(label : String = "", &action : ->) : Benchmark::IPS::Entry |
src/benchmark/ips.cr
Outdated
| @cycles = (iterations / duration.total_milliseconds * 100).to_i | ||
| @cycles = 1 if cycles <= 0 | ||
| end | ||
|
|
||
| def calculate_stats(samples) : Nil | ||
| def calculate_stats(samples : Array(Float64) | Array(Int32)) : Nil |
There was a problem hiding this comment.
thought: This looks like an arbitrary choice of two numeric types for the array element type. Theoretically, any number type should be acceptable there. But that's impossible to express in the current type grammar. We could only use Array as a generic type.
Btw. Array(Int32) actually comes from a unit test for this method. When using the higher-level API, the type is always Array(Float64).
This method (and set_cycles as well) also looks very much like a helper method and does not seem very useful as a public method. Probably the entire Entry type is just an implementation detail and should not even be exposed in the API docs.
I'd suggest leaving these questionable out for now, and undocument Entry in a separate PR.
This is the output of compiling cr-source-typer and running it with the below incantation:
This is related to #15682 .