andyferris / Dictionaries.jl

An alternative interface for dictionaries in Julia, for improved productivity and performance
Other
282 stars 28 forks source link

Ensure broadcasted functions retain their bounds checks #64

Closed c42f closed 3 years ago

c42f commented 3 years ago

The inbounds assertion within the map! implementation, combined with propagate_inbounds on gettokenvalue(d::BroadcastedDictionary, t) leads to removing the bound check on any user-defined function which is broadcasted. This adds an extra layer of dispatch to protect the user-defined function which is broadcasted.

Fixes #63

c42f commented 3 years ago

Note: the original intent for _f(d) was to be an alias for d.f in the case that I overload getproperty on AbstractDict (as a shortcut for dictionaries with Symbol keys) so perhaps it could be renamed

Yeah I figured, but I was lazy :laughing:. I renamed it to _call_f and kept _f as it was.

Also fixed the tests by making sure only is defined even on older versions of Julia.

andyferris commented 3 years ago

Great! 👍

There seems to be an UndefVarError in the CI.

c42f commented 3 years ago

UndefVarError in the CI

This was weird. I think it may have been something to do with only being defined in an inner scope in an if block.

Anyway, it turns out that those tests always passed regardless because CI runs with --bounds-check=yes. So I move that test into its own file which can be run without the --bounds-check argument.

codecov[bot] commented 3 years ago

Codecov Report

Merging #64 (7e96aba) into master (6918361) will decrease coverage by 0.04%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   74.06%   74.02%   -0.05%     
==========================================
  Files          19       19              
  Lines        1720     1721       +1     
==========================================
  Hits         1274     1274              
- Misses        446      447       +1     
Impacted Files Coverage Δ
src/broadcast.jl 50.76% <50.00%> (-0.80%) :arrow_down:

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 6918361...7e96aba. Read the comment docs.

andyferris commented 3 years ago

Anyway, it turns out that those tests always passed regardless because CI runs with --bounds-check=yes. So I move that test into its own file which can be run without the --bounds-check argument.

Duh! :man_facepalming: Nice solution.

c42f commented 3 years ago

Nice solution.

I stole it from Base :laughing:

i think it might be nicer if the whole set of tests ran in CI both with and without --bounds-check=yes, but it turns out this isn't so easy to do without a certain amount of hacking. https://github.com/julia-actions/julia-runtest/pull/46 should make it a lot easier.