WIP - remove scanf parsing in GPX date parser #1109
Conversation
Quit scanning the time string for '.' and special handling it as a string. More experimental and marked as #if 0 for easy testing, move all the scanf parsing to QRegExp. I don't know if that's wise for CPU/memory costs, but it eliminates all the scanf stuff and the entire reason to convert the incoming QString to a C String.
Coverage summary from CodacyMerging #1109 (3450e58) into
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
| if (fsec) { | ||
| dt = dt.addMSecs(lround(fsec * 1000)); | ||
| } | ||
| dt = QDateTime(date, time, Qt::UTC); | ||
|
|
||
| // Any offsets that were stuck at the end. | ||
| dt = dt.addSecs(-off_sign * off_hr * 3600 - off_sign * off_min * 60); |
There was a problem hiding this comment.
I like the replacement of sscanf with QRegularExpression, and elimination of cstdio, cstring.
I had WIP, including a bunch of issues that appear with Qt 6.5.0, with an idea regarding offsets. Reviewing your ideas and incorporating mine got messy, a modified version to compare is attached. To use this also change the declaration in defs.h adding a default to match current code.
gpsbabel::DateTime xml_parse_time(const QString& dateTimeString, bool local_is_utc = true);
I modified the regular expressions based on https://www.w3.org/TR/xmlschema-2/#dateTime. I didn't fret over negative years, or years beyond 9999.
I do worry a bit about the offset regular expression matching something it shouldn't. I haven't thought of a way it could mismatch something besides a timezone from https://www.w3.org/TR/xmlschema-2/#isoformats
qRound and qRound64 let you round a double to a int or qint64.
It also got messy with the conditional code and keeping them both running, I did the lazy thing and chopped it.
I haven't tried the enhanced kml tests to see if they work with these changes.
I attached the modified gpx.cc, or a patch for both gpx.cc and defs.h:
gpx.cc.gz
xsddatetime.patch
There was a problem hiding this comment.
I knew we were about to collide. That's why I RFC'ed this to you before hand. The #if 0 mess was just to mark my before and after. It was on the chopping block for after your vague agreement that this was a sane thing to do anyway.
The regexes will definitely match theoretical things they shouldn't. But those things shouldn't be valid ISO/GPX/KML times. We could have fooled scanf, too.
I'll take your version of this, fix the rounding issues pushing us over a second and then calling Time.addSec() with the rest, and run with it.
Thanx for talking it through.
|
please merge in "add testing for kml:dateTimeType" #1112, which has been merged with the master branch. That may keep us honest. |
|
BTW https://regex101.com/ is handy for manual testing of these PCREs, especially if you use raw string literals so you can just past them in. |
|
There is an issue with my suggestion to round msecs. If we round .9999999 secs up to 1000 msecs it will be out of the legal range for QTime. If we set msecs to zero and increment secs it may be out of range ... |
|
I'd just truncate the milliseconds. that is what I just caught in unicsv.cc. adding a second in anything but utc (or utcoffset) is expensive, it can require a double time zone conversion. |
|
Whats a worse case truncation error of 0.5milliseconds among friends? osm just convinced me to drop all the fractional seconds! I don't think its worth the agony of dealing with it. |
|
Without that round, there's a whole lot of test suite that fails.
I developed this, much like I do everything, running
make && ./testo unicsv (or whatever one test gives me the fastest coverage)
Then, smug in a job obviously well done, I ran the full testo and watched
it combust. I saw everything was off by one digit in the smallest location
and realized that rounding was needed.. Well, I don't know if it's NEEDED,
but it's what we had. Whatever format I was using didn't care about the
round, but enough other things in our suite (very likely produced by us...)
relied on it that I carried it forward.
Can we round it unless the round would result in > 1 sec?
In the QTime examples <https://doc.qt.io/qt-6/qtime.html#addSecs>,
addSecs(70) is documented to work, why shouldn't addMSecs(1001) ? But even
if it is a bug, I guess we have to live with it until the last Linux distro
updates, which would likely be about Sun's office furniture closeout sale
from the previous PR.
Rounding a .9985 up, but not rounding a fractional .9999 up wouldn't be the
worst possible thing to do here, though it IS a bit tacky. Someone's
supersonic drone computations may have a bump in them, but I think I can
live with that. What do you think?
…On Sun, May 14, 2023 at 7:59 PM tsteven4 ***@***.***> wrote:
Whats a worse case truncation error of 0.5milliseconds among friends? osm
just convinced me to drop all the fractional seconds! I don't think its
worth the agony of dealing with it.
—
Reply to this email directly, view it on GitHub
<#1109 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC3VAD25EQPQDWZ2P6KL4CDXGF5WBANCNFSM6AAAAAAYA77SQA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
The rounding effects on test are all too believable, but on my system applying the patch below to the patch above doesn't cause any test failures. But there is a history of needing rounding to get tests to work across platforms, but usually that was windows vs. the rest of the world. |
|
same change as above wrt rounding -> truncation, no test failures on macos either. |
Quit scanning the time string for '.' and special handling it as a string.
More experimental and marked as #if 0 for easy testing, move all the scanf parsing to QRegExp. I don't know if that's wise for CPU/memory costs, but it eliminates all the scanf stuff and the entire reason to convert the incoming QString to a C String.
@tsteven4 , I know you've fretted a lot about GPX date parsing recently. Is this overall direction better or worse? There are some additional cleanups this would enable, but this allows for easy flipping between old and new for now.