Overflow prepare stdlib#7256
Conversation
Removing any of these prevent grisu3_specs to pass with overflow semantics
Otherwise backtraces were not shown on failures
| attributes.clear | ||
|
|
||
| if abbrev = abbreviations[code - 1]? # abbreviations.find { |a| a.code == abbrev } | ||
| if abbrev = abbreviations[code &- 1]? # abbreviations.find { |a| a.code == abbrev } |
There was a problem hiding this comment.
There should be heavily TODO'd, or compiled into an issue. If we just do a blanket "everything which fails due to overflow gets the old semantics" we miss a fantastic opportunity to get rid of bugs!
| @@ -95,7 +95,7 @@ struct Pointer(T) | |||
| # TODO: If throwing on overflow for integer conversion is implemented, | |||
| # then (here and in `Pointer#-`) for a `UInt64` argument the call to | |||
| # `to_i64` should become `as_unsafe`. | |||
There was a problem hiding this comment.
This comment should have been replaced with an explanation.
|
This is a huge missed opportunity. The non-obvious cases where unchecked arithmetic is used must be accompanied by a comment, and all the uses must be audited. I have no confidence that if we say we'll do those bugfixes and refactors "after we've implemented overflow", that they'll ever get done. There at the very least needs to be a high-priority issue enumerating these cases, and actively working on fixing them. And this issue needs to be completed before 0.27.1. Silently copying the existing behaviour is very dangerous. |
|
I do want to refactor but we can't for example refactor Time to use overflow until overflow is the default behavior (or we need to have a huge macro if to use overflow if enabled). I think that is better to than after. Other cases like dwarf requires some more digging and I wasn't able to dig deeper without further time. At worst, we are emitting the same code as before. Not worse. I expect that there are some more missing parts of the std lib that will need s/+/&+/ but were not detected with the current spec coverage. This PR is a whole TODO after overflow is turned on IMO. But doing it before will requiere code duplication or efforts that can happen later and improve stdlib once we have more a solid ground. |
|
@bcardiff I'm fine with leaving time, since it has a comment. I'm fine with leaving DWARF too, but it needs a comment or an issue or something to track this technical debt which has just been created. JSON and YAML should be refactored before 0.27.1. |
|
Have we done any of these refactors? |
3 participants
The PR updates the stdlib to use unchecked operations & conversions where needed.
The changes should be read as: keep the current semantic once the operations will default to overflowing.
Some changes direct make sense because they manipulate data as bits mainly (eg: hasher).
Some might look a bit odd (eg: dwarf, floatprinter). The changes were required to make specs pass once the overflow is turned on.
Other changes can be revised in future version (eg: time) since there is manual detection of overflow that could be simpler in the future.