Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce OptionString, #1352

Merged
merged 45 commits into from
Oct 30, 2024
Merged

introduce OptionString, #1352

merged 45 commits into from
Oct 30, 2024

Conversation

tsteven4
Copy link
Collaborator

a replacement for OptionCString. Formats and filters can be manually converted, with the potential to eliminate c character string usage. In contrast to OptionCString, OptionString can be implicitly cast to a bool or to a const QString&, but not a const char*.

OptionCString::get() usages can often use an implicit cast from OptionString instead, although the usage of get() is still valid.

By using QptionString::get() any of the QString functions that accept a const QString& can be used such as QString::toInt(),, QString::toDouble(), ...

a replacement for OptionCString. Formats and filters can be
manually converted, with the potential to eliminate c character
string usage.
OptionCString::get() usages can often use an implicit cast from
OptionString instead, although the usage of get() is still valid.
Copy link
Collaborator

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seven minutes from merge to turndown is some kind fo a new record. :-)

Darned if that's not a lot cleaner, though.

Since OCS didn't actually go away, I assume there's more. Go for it!

@tsteven4
Copy link
Collaborator Author

Since OCS didn't actually go away, I assume there's more.

There are random usages of c strings, for example xml_parse_time, KmlFormat::kml_lookup_gc_icon, ... . One intent here is to enable the removal of c strings required by option handling. That alone requires manual intervention in every format and filter that use non ARGTYPE_BOOL options. At least we are set up to do this one format/filter at a time as energy allows. I was encouraged that this already is reducing our dependency on , , . And I agree, cleaner code is a nice benefit as well.

@PacketFiend
Copy link
Contributor

Hey fellas,

Wondering if anything more needs to be done with the IGC bits because of this? It does make pretty heavy use of options...

@tsteven4
Copy link
Collaborator Author

@PacketFiend I have related changes for igc on deck.

overloads, as opposed to default parameters, allow tools to find differen usages.

improved OptionString error messages with module and argstring information.
to indicate integer base and if trailing data is allowed.
Use these fields to check and convert integer/doubles in Vecs, fataling on errors.
and when necessary convert these from ARGTYPE_STRING to ARGTYPE_FLOAT.
@tsteven4
Copy link
Collaborator Author

At this point OptionString::toInt and ::toDouble will fatal if there is a conversion error, and a non-null status argument was not passed. This is by far the most common use case. This might leave a ff_type_serial device in an intermediate state. This risk can be avoided by adding strict validation of ARGTYPE_INT and ARGTYPE_FLOAT in Vecs. This was accomplished by adding characterization for options of ARGTYPE_INT and ARGTYPE_FLOAT. For integers we need to know the base (automatic, 10, 16). For integers and floats we need to know if trailing data is allowed, typically used to allow units to be entered along with the value.

I plan to add OptionInt and OptionFloat corresponding to ARGTYPE_INT and ARGTYPE_FLOAT, and add methods to those that retrieve the value converted by Vecs instead of converting again.

The overloads of OptionString::toInt and ::toDouble allow the unusual use cases to be identified by a tool such as JetBrains ReSharper. This was used to help validate the proper introduction of the characterization bits in argtype.

One wrinkle is parse_distance and parse_speed which convert a QString, not an Option*. Callers may or may not get that string from an Option*. These cases have all been examined, the characterization bit to allow trailing data set in argtype, and those that were ARGTYPE_STRING have been changed to ARGTYPE_FLOAT.

@tsteven4
Copy link
Collaborator Author

I plan to add OptionInt and OptionFloat corresponding to ARGTYPE_INT and ARGTYPE_FLOAT, and add methods to those that retrieve the value converted by Vecs instead of converting again.

This has been done. All the conversions of ARGTYPE_INT and ARGTYPE_FLOAT are done in vecs, although they may be done again in parse_distance or parse_speed.

This is ready for some serious review.

The GUI validator used with ARGTYPE_INT and ARGTYPE_FLOAT will
reject input with any trailing data.

The CLI, including Vecs::assign_option, keys off the Option subclass now,
not the ARGTYPE.

Note that OptionInt and OptionDouble still allow trailing data.  If
trailing data is allowed with these classes we just pair these with
ARGTYPE_STRING which controls validation in the GUI.
delete unused macro
delete extraneous file
@tsteven4 tsteven4 merged commit 0d8f0f9 into GPSBabel:master Oct 30, 2024
17 of 18 checks passed
@tsteven4 tsteven4 deleted the optionstring branch October 30, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants