remove conflicting default call in tables.getOrDefault
#24265
Conversation
metagn
changed the title
default call in tables.getOrDefaultdefault call in tables.getOrDefault
| @@ -434,7 +434,6 @@ proc getOrDefault*[A, B](t: Table[A, B], key: A, default: B): B = | |||
| let a = {'a': 5, 'b': 9}.toTable | |||
| doAssert a.getOrDefault('a', 99) == 5 | |||
| doAssert a.getOrDefault('z', 99) == 99 | |||
| result = default(B) | |||
There was a problem hiding this comment.
No. Instead the parameter default: B should be def: B.
There was a problem hiding this comment.
Did both, and did it for all parameters named default in tables (OrderedTable had the same bug). There are others across the codebase but they're less harmful and this PR being simpler is better.
Unfortunately this breaks named argument usage (i.e. getOrDefault(foo, bar, default = val)). We can change the code in this repo but it might still break packages.
There was a problem hiding this comment.
Yeah I know but it's a risk I'm willing to take.
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
) fixes CI after #24265, the CI passed in the original PR somehow
fixes #23587
As explained in the issue,
getOrDefaulthas a parameter nameddefaultthat can be a proc after generic instantiation. But the parameter having a proc type overrides all other overloads including the magicsystem.defaultoverload and causes a compile error if the proc doesn't match the normal use ofdefault. To fix this, theresult = default(B)initializer call is removed because it's not needed,resultis always set ingetOrDefaultImplwhen a default value is provided.This is still a suspicious behavior of the compiler but
tablesworking has a higher priority.