WIP - remove scanf parsing in GPX date parser

[robertlipe]

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.

[codacy-production[bot]]

Coverage summary from Codacy

Merging #1109 (3450e5846ff29fd384f5498326f8e6a1941ad620) into master (9dccbfc572398be2e876bcfd02a46c91056113e3) - See PR on Codacy

Coverage variation	Diff coverage
0.00%	100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (9dccbfc572398be2e876bcfd02a46c91056113e3)	24154	16373	67.79%
Head commit (3450e5846ff29fd384f5498326f8e6a1941ad620)	24152 (-2)	16371 (-2)	67.78% (0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1109)	18	18	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[tsteven4]

please merge in "add testing for kml:dateTimeType" #1112, which has been merged with the master branch. That may keep us honest.

[tsteven4]

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.

[tsteven4]

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 ...

[tsteven4]

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.

[tsteven4]

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.

[GPSBabelDeveloper]

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 https://github.com/GPSBabel/gpsbabel/pull/1109#issuecomment-1547055799, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3VAD25EQPQDWZ2P6KL4CDXGF5WBANCNFSM6AAAAAAYA77SQA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[tsteven4]

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.

$ diff gpx.cc.p1 gpx.cc
425c425
<     int msec = qRound(1000.0 * match.captured(7).toDouble());
---
>     int msec = 1000.0 * match.captured(7).toDouble();

[tsteven4]

same change as above wrt rounding -> truncation, no test failures on macos either.

[tsteven4]

BTW, a similar change to truncate ms in UnicsvFormat::unicsv_parse_time didn't cause any failures on any platforms in CI. The base for test this was what is about to replace #1103 and #1106.