Tweak uses of RegExp and parsing using them.#1097
Conversation
|
|
||
| mixin TimestampMixin { | ||
| static final RegExp finalGroupsOfThreeZeroes = RegExp(r'(?:000)*$'); | ||
| static final RegExp finalGroupsOfThreeZeroes = RegExp(r'(?:000)+$'); |
There was a problem hiding this comment.
(Only used for replacing with '', no need to match an empty string.)
| target.seconds = Int64((micros / Duration.microsecondsPerSecond).floor()); | ||
| target.nanos = (micros % Duration.microsecondsPerSecond).toInt() * 1000; | ||
| var micros = dateTime.microsecondsSinceEpoch; | ||
| var seconds = micros ~/ Duration.microsecondsPerSecond; |
There was a problem hiding this comment.
Going through doubles loses precision for large/small dates. Tests added.
| var micros = dateTime.microsecondsSinceEpoch; | ||
| var seconds = micros ~/ Duration.microsecondsPerSecond; | ||
| micros = micros.remainder(Duration.microsecondsPerSecond); | ||
| if (micros < 0) { |
There was a problem hiding this comment.
(Usual way to handle ~//remainder not working like %.)
| var nanos = 0; | ||
| final Match? fracSecsMatch = RegExp(r'\.(\d+)').firstMatch(json); | ||
| if (fracSecsMatch != null) { | ||
| var jsonWithoutFracSec = json.replaceFirstMapped(RegExp(r'\.(\d+)'), ( |
There was a problem hiding this comment.
(Could combine match and replace.)
| final seconds = duration.seconds; | ||
| final nanos = duration.nanos; | ||
| // Extra sign needed if `seconds` is zero and nanos is negative. | ||
| final sign = seconds.isZero && nanos < 0 ? '-' : ''; |
There was a problem hiding this comment.
This was a bug. It wrote 0 seconds and -500000000 nanos as 0.5.
| duration.seconds = seconds; | ||
| final nanos = int.parse((match[2] ?? '').padRight(9, '0')); | ||
| duration.nanos = seconds < 0 ? -nanos : nanos; | ||
| final isNegative = secondsString.startsWith('-'); |
There was a problem hiding this comment.
This was a bug in the opposite direction.
It parsed -0.5 as 0 seconds and 500000000 nanos, and didn't negate the nanos since 0 was not negative.
| part of '../protoc.dart'; | ||
|
|
||
| final RegExp _dartIdentifier = RegExp(r'^\w+$'); | ||
| final RegExp _dartIdentifier = RegExp(r'^[a-zA-Z_]\w*$'); |
There was a problem hiding this comment.
That didn't match Dart identifiers, it allowed starting with 0.
(Also still doesn't allow $s, but I assume that's deliberate.)
| multiLine: false, | ||
| caseSensitive: false, | ||
| static final RegExp _decimalLiteralRegex = RegExp( | ||
| r'^[+\-]?(\d*\.)?\d+([eE][+\-]?\d+)?$', |
There was a problem hiding this comment.
Instead of three regexpsm just use one and switch on whether it has double-literal parts.
| class ProtobufField { | ||
| static final RegExp _hexLiteralRegex = RegExp( | ||
| r'^0x[0-9a-f]+$', | ||
| multiLine: false, |
There was a problem hiding this comment.
multiLine: false is the default.
(And I think being explicit is more readable than the caseSensitive: false flag.)
| var lines = LineSplitter.split(value).toList(); | ||
| var lines = LineSplitter.split(value) | ||
| .map((line) => line.trimRight()) | ||
| .toList(); |
There was a problem hiding this comment.
Doing trimRight early makes later tests easier, it turns all lines with only whitespace into empty lines.
| for (var i = 0; i < lines.length; i++) { | ||
| var line = lines[i]; | ||
| if (line.isNotEmpty) lines[i] = line.substring(prefix.length); | ||
| } |
There was a problem hiding this comment.
Just update the list in-place instead of building a new one.
| bool _isDartFieldName(String name) => name.startsWith(_dartFieldNameExpr); | ||
|
|
||
| final _dartFieldNameExpr = RegExp(r'^[a-z]\w+$'); | ||
| final _dartFieldNameExpr = RegExp(r'^[a-z]\w*$'); |
There was a problem hiding this comment.
(Seems unlikely that single-letter fields are not allowed, but two-letter ones are.)
| if (e is FormatException) { | ||
| final pathExpression = | ||
| RegExp(r'root(\["[^"]*"]*\])*').firstMatch(e.message)![0]!; | ||
| RegExp(r'root(?:\["[^"]*"\])*').firstMatch(e.message)![0]!; |
There was a problem hiding this comment.
(I think the extra ]* here was a typo. If it had been deliberate, it would have been escaped.
Since it matches the empty string, nobody noticed that you could match root["a"]]]]].
| var fractionalSeconds = match[2]; | ||
| var nanos = 0; | ||
| if (fractionalSeconds != null) { | ||
| if (fractionalSeconds.length > 9) { |
There was a problem hiding this comment.
The original code would parse 0.9999999999 as 9+ seconds.
I assume the input would be correctly generated and not have more than 9 digits, but better to be safe.
(Could also throw.)
1 participant
Looked at the RegExps and how they are used and made some tweaks and fixes.
Fixed some actual bugs too: