feat: Add testsuite validation for option types by robertlipe · Pull Request #1473 · GPSBabel/gpsbabel

robertlipe · 2025-10-07T23:54:43Z

feat: Add runtime validation for option types
Revert "feat: Add runtime validation for option types"
feat: Add runtime validation for option types

Add a `get_type()` virtual method to the `Option` class hierarchy to enable runtime type identification. Use this new method in `Vecs::validate_args` to validate that the `argtype` specified in `arglist_t` tables matches the actual type of the `Option` object. This will catch configuration errors at startup, such as the one that was present in the kalman filter. This change also includes several fixes to the `Option` class hierarchy to resolve build errors that surfaced during development, including breaking a circular dependency between `defs.h` and `option.h`.

This reverts commit 6b3661c.

codacy-production · 2025-10-07T23:57:02Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -0.23%	✅ 21.38%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`fb56e8a`)	27512	17189	62.48%
Head commit (`72ca40c`)	27652 (+140)	17213 (+24)	62.25% (-0.23%)

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 (#1473)	145	31	21.38%

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

I can't recreate the original problem as history has been erased by forced pushes. I think the original code should work, but evidently there was a hole?

tsteven4 · 2025-10-08T00:17:59Z

  virtual void init(const QString& id) {}
  virtual void reset() = 0;
  virtual void set(const QString& s) = 0;
+  virtual bool isValid(const QString& s) const = 0;


shouldn't isValid be [[nodiscard]]? (everywhere).

tsteven4 · 2025-10-08T00:48:58Z

+          }
+        }
+      }
+


Aren't these cases covered already by the original code below which uses a dynamic cast to deduce the type? I'm missing something or this whole PR seems redundant. I'm trying to find the original failure but with the forced pushes I'm having trouble seeing what the problem was.

Indeed. When we (Gemini and I) were adding new options in kalman.cc, we triggered a crash from a mismatched type. It was somethin glike the difference in the arguments between strings and floats (sometimes we "atof" and sometimes we ust want a number) and things would get ugly. We fixed the caller, but it occurred to. methat we should be able to catch it (when I started, I was thinking at startup and that probably stuck in my head) but we indeed learned it's in the test - which is cool! This is kind of how I got in trouble because i started two-timing the kalman work and this work at the same time and I think that having two PR-to-bes in one branch confuses it WAY more that it screws me up. I can USUALLY pull it off, but you can tell that I've spent the evening pullin the Kalman work apart into manageable PRs. I DOSes myself. :-/

It's also indeed horse-crap that it chose to delete everything checked in and rewrite git history, nuking a whole lot of history each at least once. SO maddening.

So this PR can be abandoned and closed, correct?

tsteven4 · 2025-10-08T01:03:33Z

BTW the validation is not at startup, but when you run the validate_formats test.
./testo -p bld/gpsbabel validate_formats

tsteven4 · 2025-10-08T01:15:44Z

If I take kalman-2, restore options.h and options.cc from main, take out the new check code in vecs.cc so only the original checks exist, and then introduce type mismatches in kalman.h:

-  OptionDouble gap_factor_option_;
+  OptionInt gap_factor_option_;

and

     {"r_scale", &r_scale_option_, "Measurement noise covariance scaling factor",
-      "1.0", ARGTYPE_FLOAT, ARG_NOMINMAX, nullptr
+      "1.0", ARGTYPE_INT, ARG_NOMINMAX, nullptr

the the test shows:

Running validate_formats.test
main: "kalman" OptionInt option without trailing data "gap_factor" is not of ARGTYPE_INT.
main: "kalman" Int option "gap_factor" default value "10.0" is not an integer.
main: "kalman" OptionDouble without trailing data "r_scale" is not of ARGTYPE_FLOAT.
bld/gpsbabel returned error 1

tsteven4 · 2025-10-08T01:53:09Z

I see some activity in the kalman-2 branch that shows the kalman options were originally ARGTYPE_STRING instead of ARGTYPE_FLOAT. If I

take the kalman-2 branch and
restore vecs.cc, option.h and option.cc from master,
and change ARGTYPE_FLOAT -> ARGTYPE_STRING in kalman.h to create the mismatches
and run the test the mismatches are detected:

tsteven4@PEDALDAMNIT:~/work/kalman2$ ./testo -p bld/gpsbabel validate_formats
Running validate_formats.test
main: "kalman" OptionDouble without trailing data "gap_factor" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "r_scale" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "q_scale_pos" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "q_scale_vel" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "max_speed" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "interp_max_dt" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "interp_min_multiplier" is not of ARGTYPE_FLOAT.
bld/gpsbabel returned error 1

Add a `get_type()` virtual method to the `Option` class hierarchy to enable runtime type identification. Use this new method in `Vecs::validate_args` to validate that the `argtype` specified in `arglist_t` tables matches the actual type of the `Option` object. This will catch configuration errors at startup. This change also includes several fixes to the `Option` class hierarchy to resolve build errors that surfaced during development, including breaking a circular dependency between `defs.h` and `option.h`.

tsteven4 · 2025-10-08T01:56:10Z

I see some activity in the kalman-2 branch that shows the kalman options were originally ARGTYPE_STRING instead of ARGTYPE_FLOAT. If I

take the kalman-2 branch and

restore vecs.cc, option.h and option.cc from master,

and change ARGTYPE_FLOAT -> ARGTYPE_STRING in kalman.h to create the mismatches

and run the test the mismatches are detected:
tsteven4@PEDALDAMNIT:~/work/kalman2$ ./testo -p bld/gpsbabel validate_formats
Running validate_formats.test
main: "kalman" OptionDouble without trailing data "gap_factor" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "r_scale" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "q_scale_pos" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "q_scale_vel" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "max_speed" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "interp_max_dt" is not of ARGTYPE_FLOAT.
main: "kalman" OptionDouble without trailing data "interp_min_multiplier" is not of ARGTYPE_FLOAT.
bld/gpsbabel returned error 1

@robertlipe My conclusion is the original option checking was sufficient to detect the errors you had. I suspect that you assumed the validation ran at startup instead of during the validate_formats test (which also validates the filter arguments).

tsteven4 · 2025-10-08T01:58:29Z

@robertlipe note that catching any mismatches at test in CI prevents any mismatches from being merged through a PR, but it doesn't prevent them from confusing a developer who hasn't run the test yet.

codacy-production · 2025-10-08T02:16:50Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01%	✅ 67.86%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`fb56e8a`)	27512	17189	62.48%
Head commit (`2f540bb`)	55091 (+27579)	34425 (+17236)	62.49% (+0.01%)

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 (#1473)	28	19	67.86%

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

robertlipe added 2 commits October 7, 2025 18:08

Revert "feat: Add runtime validation for option types"

0db198a

This reverts commit 6b3661c.

tsteven4 reviewed Oct 8, 2025

View reviewed changes

Comment thread gui/coretool/core_strings.h

tsteven4 reviewed Oct 8, 2025

View reviewed changes

robertlipe force-pushed the kalman3 branch 2 times, most recently from 034ebfa to 61d9235 Compare October 8, 2025 01:34

robertlipe changed the title ~~kalman3~~ feat: Add runtime validation for option types Oct 8, 2025

robertlipe force-pushed the kalman3 branch 2 times, most recently from 5ea736b to de5ac3d Compare October 8, 2025 01:41

robertlipe force-pushed the kalman3 branch from de5ac3d to 2f540bb Compare October 8, 2025 01:54

robertlipe changed the title ~~feat: Add runtime validation for option types~~ feat: Add testsuite validation for option types Oct 8, 2025

Navigation Menu

feat: Add testsuite validation for option types - #1473

feat: Add testsuite validation for option types#1473
robertlipe wants to merge 3 commits into
masterfrom
kalman3

robertlipe commented Oct 7, 2025

Uh oh!

codacy-production Bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

tsteven4 left a comment

Uh oh!

Uh oh!

tsteven4 Oct 8, 2025

Uh oh!

Uh oh!

tsteven4 Oct 8, 2025

Uh oh!

robertlipe Oct 8, 2025

Uh oh!

tsteven4 Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025 •

edited

Loading

Uh oh!

codacy-production Bot commented Oct 8, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robertlipe commented Oct 7, 2025

Uh oh!

codacy-production Bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Uh oh!

Uh oh!

tsteven4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025

Uh oh!

tsteven4 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codacy-production Bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codacy-production Bot commented Oct 7, 2025 •

edited

Loading

tsteven4 commented Oct 8, 2025 •

edited

Loading

codacy-production Bot commented Oct 8, 2025 •

edited

Loading