fix unicsv reader truncation differences on i386/debian

[tsteven4]

with 1.9.0 debian had some test failures on i386. in the unicsv test with reference unicsv_subsecond.csv 13:20:20.060 was echoed as 13:20:20.059. 13:20:21.600 was echoed as 13:20:21.599.

The same bug also caused a failure in the track test with reference utm_subsecond_track~csv.csv.

These variations aren't serious, but this PR uses an integer arithmetic conversion instead of a floating point arithmetic to avoid any variation in the intentional truncation to milliseconds.

I also see an error in the nmea test with reference nmeadatetime.csv. This is a rounding difference. It is not fixed. I also see an error in the xcsv test with reference datetime~xcsv.xcsv. This is a Y2038 bug. It is not fixed.

[codacy-production[bot]]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
+0.00%	100.00%

Coverage variation details

| | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (c95e1cfc2331f5e22dcae51ee637b70ce9973cb9) | 23128 | 16018 | 69.26% | | | Head commit (e5b3093bd92bd665111b22bd76c3bf3697f7d71e) | 23130 (+2) | 16020 (+2) | 69.26% (**+0.00%**) | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `

Diff coverage details

| | Coverable lines | Covered lines | Diff coverage | | ------------- | ------------- | ------------- | ------------- | | Pull request (#1201) | 4 | 4 | **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: `/ * 100%`

See your quality gate settings    Change summary preferences

[robertlipe]

I am, of course, OK with the proposal if you like it better. I can sweep back with the second form.

But I'm 95% sure I've read that Y2038 issues is one of the reasons that 32-bit support in Linux in general is going away - the issues are so deep and there are so few people left that it's predicted to be removed instead of fixed. So I'm not sure I see any needed heroics there.

On Fri, Oct 27, 2023 at 7:19 PM codacy-production[bot] < @.***> wrote:

Coverage summary from Codacy See diff coverage on Codacy https://app.codacy.com/gh/GPSBabel/gpsbabel/coverage/pull-requests/1201?utm_source=github.com&utm_medium=coverageSummary&utm_campaign=coverageSummaryPullRequest Coverage variation Diff coverage +0.00% 100.00% Coverage variation details Coverable lines Covered lines Coverage Common ancestor commit (c95e1cf https://github.com/GPSBabel/gpsbabel/commit/c95e1cfc2331f5e22dcae51ee637b70ce9973cb9 ) 23128 16018 69.26% Head commit (e5b3093 https://github.com/GPSBabel/gpsbabel/commit/e5b3093bd92bd665111b22bd76c3bf3697f7d71e ) 23130 (+2) 16020 (+2) 69.26% (+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> - Diff coverage details Coverable lines Covered lines Diff coverage Pull request (#1201 https://github.com/GPSBabel/gpsbabel/pull/1201) 4 4 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>/ * 100% See your quality gate settings https://app.codacy.com/gh/GPSBabel/gpsbabel/settings/quality?utm_source=github.com&utm_medium=coverageSummary&utm_campaign=coverageSummaryGates Change summary preferences https://app.codacy.com/gh/GPSBabel/gpsbabel/settings/integrations?utm_source=github.com&utm_medium=coverageSummary&utm_campaign=coverageSummarySettings

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1201#issuecomment-1783632567, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3ZC3FOJAVLMJTALAP3YBRFQ3AVCNFSM6AAAAAA6TQOBHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBTGYZTENJWG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[tsteven4]

It sure is cleaner, but we have been burned with QTime::fromString before, but apparently not from Qt::ISODateWithMs although I thought I had tried that back then.

https://github.com/GPSBabel/gpsbabel/blob/ae4ac826a90bc4112d98854e397f1abacd3e811e/nmea.cc#L351-L355

Does section(" ", 0, 0) do anything?

I do see test changes, I added the last one "22,33.784805,-117.474327,"WPT022",2014/09/17,23:59:59.999999999999". So it appears QTime::fromString may be rounding, but is smart enough not to round to an illegal QTime.

Running unicsv.test
--- ./reference/unicsv_subsec~csv.csv   2023-10-28 05:45:52.092839226 -0600
+++ /tmp/gpsbabel.2699/unicsv_subsec.csv        2023-10-28 05:52:58.487210034 -0600
@@ -17,7 +17,8 @@
 16,33.784805,-117.474327,"WPT016",2014/09/17,13:20:23
 17,33.784805,-117.474327,"WPT017",2014/09/17,13:20:23.690
 18,33.784805,-117.474327,"WPT018",2014/09/17,13:20:23.699
-19,33.784805,-117.474327,"WPT019",2014/09/17,13:20:23.699
-20,33.784805,-117.474327,"WPT020",2014/09/17,13:20:23.699
-21,33.784805,-117.474327,"WPT021",2014/09/17,13:20:23.699
-22,33.784805,-117.474327,"WPT022",2014/09/17,13:20:23.699
+19,33.784805,-117.474327,"WPT019",2014/09/17,13:20:23.700
+20,33.784805,-117.474327,"WPT020",2014/09/17,13:20:23.700
+21,33.784805,-117.474327,"WPT021",2014/09/17,13:20:23.700
+22,33.784805,-117.474327,"WPT022",2014/09/17,13:20:23.700
+23,33.784805,-117.474327,"WPT022",2014/09/17,23:59:59.999

[tsteven4]

QTime::fromString is quite complicated, rounding, watching for rounding up to cause an invalid QTime, potentially limiting at 23:59:59.999 or rolling over into the next day. https://github.com/qt/qtbase/blob/6.2.4/src/corelib/time/qdatetime.cpp#L2182-L2211

[tsteven4]

@robertlipe some questions:

The unicsv date and time parsing came in with https://github.com/GPSBabel/gpsbabel/commit/e3f54a668, and has a tortured 24 year history of rounding problems.

1. Isn't the format specifier %1[-.//] the same as %1[-./]? (unicsv_parse_date) We are not escaping a backslash.
2. Isn't the format specifier %1[.://] the same as %1[.:/]? (unicsv_parse_time) We are not escaping a backslash.
3. I can imagine a date using a '-', '.', '/' as a separator, e.g. 10.11.2022 or 10-11-2022 or 10/11/2022, so those three separators make some sense. But I have a harder time imaging a time using '.' or '/' as a separator, e.g. 12.10.55 or 12/10/55, wouldn't everyone use 12:10:55 if they used a separator?

[tsteven4]

https://www.ibm.com/docs/en/i/7.3?topic=80-timsep-time-separator-keyword-display-files suggests time separators of colon, period, comma or blank.

[tsteven4]

https://docs.oracle.com/cd/E19455-01/806-0169/overview-6/index.html suggests time separators of a colon or period.

[GPSBabelDeveloper]

I certainly won't hard sell it. It was just the reading that regex made my head hurt. Your subsequent read of it made it hurt more.

Simplified() was to fix one case with a leading space that failed. Split() was to fix a case passed in with a trailing " AM".

I'm fine with the original - or anything else you bake - I was just pointing out this had been a problem for us in the past and that we had the opportunity to replace it with a simpler looking fragments that surely has NEW and exciting problems.

I'm fine with taking your change and never thinking about this again.

On Sat, Oct 28, 2023, 8:30 AM tsteven4 @.***> wrote:

https://docs.oracle.com/cd/E19455-01/806-0169/overview-6/index.html suggests time separators of a colon or period.

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1201#issuecomment-1783811067, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3VAD3G3GIO73JLOBPQXG3YBUCHLAVCNFSM6AAAAAA6TQOBHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBTHAYTCMBWG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[robertlipe]

I"m not sure about #1 vs #2, but I agree that the // is bogus. For #3, I've never seen periods used, but the original submission (from Olaf, in Germany) said they were used there. Are they common? I have no idea. Quoting the dot may be necessary to stop it from matching any single character.

I was experimenting with letting Bard write them - since I hate writing regexes AND reading them - it produced an overkill solution that's a bit less clever and therefore, IMO, a bit easier to read: https://g.co/bard/share/3c91a625a6ba I like that it gives examples of wht it will and will not read. It almost writes our test cases. This is a bit more on parity with our current parser: https://g.co/bard/share/95e0b1fc56a6 Even if we don't use that, it clarifies that [.:] (no escaping on dot needed after all) is fine.

But I really don't want to gum this up.

[tsteven4]

Are our wires crossed? How did we get from a format string to a regular expression? Bard is interesting, I have used other online regular expression checkers when I have to go there.

[tsteven4]

After looking at the Qt QDateTime/QDate/QTime::fromString code I don't feel as bad about the amount of thrashing we have done trying to get these conversions correct.

[robertlipe]

My wires are just plain wrong. Sorry. When I scanned my mail today, that scanf string locked into my brain as a regex. The syntax is vaguely similar and that's a weapon we use elsewhere, so that sort of made sense to my impaired brain.

Agreed that accepting free form, unstructured text is generally horrible.

Humans just tend to not communicate in protobufs. :-)

RJL

On Sun, Oct 29, 2023 at 5:08 PM tsteven4 @.***> wrote:

After looking at the Qt QDateTime/QDate/QTime::fromString code I don't feel as bad about the amount of thrashing we have done trying to get these conversions correct.

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1201#issuecomment-1784239344, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD36MRQCNVU3KKNM45C3YB3HWPAVCNFSM6AAAAAA6TQOBHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBUGIZTSMZUGQ . You are receiving this because you were mentioned.Message ID: @.***>