From c6b998a82e6d4a9fe8f031dda48de3312bcc826a Mon Sep 17 00:00:00 2001 From: tsteven4 <13596209+tsteven4@users.noreply.github.com> Date: Thu, 18 Aug 2022 07:06:56 -0600 Subject: [PATCH] QStringize vecs a bit more. (#913) This fixes an ancient FIXME regarding the use of the option name to indicate a option was given without a value. This enhances the validity checking of default, minimum and maximum values. --- defs.h | 12 +-- filter_vecs.cc | 20 ++--- reference/filter1.txt | 4 +- trackfilter.h | 4 +- vecs.cc | 195 +++++++++++++++++++++++++++--------------- vecs.h | 8 +- 6 files changed, 153 insertions(+), 90 deletions(-) diff --git a/defs.h b/defs.h index d555e73a3..4dfd4830a 100644 --- a/defs.h +++ b/defs.h @@ -960,13 +960,13 @@ void setshort_defname(short_handle, const char* s); #define ARG_NOMINMAX nullptr, nullptr struct arglist_t { - const char* argstring{nullptr}; - char** argval{nullptr}; - const char* helpstring{nullptr}; - const char* defaultvalue{nullptr}; + const QString argstring; + char** const argval{nullptr}; + const QString helpstring; + const QString defaultvalue; const uint32_t argtype{ARGTYPE_UNKNOWN}; - const char* minvalue{nullptr}; /* minimum value for numeric options */ - const char* maxvalue{nullptr}; /* maximum value for numeric options */ + const QString minvalue; /* minimum value for numeric options */ + const QString maxvalue; /* maximum value for numeric options */ char* argvalptr{nullptr}; /* !!! internal helper. Not used in definitions !!! */ }; diff --git a/filter_vecs.cc b/filter_vecs.cc index a83a04225..78a69a5d5 100644 --- a/filter_vecs.cc +++ b/filter_vecs.cc @@ -221,7 +221,7 @@ Filter* FilterVecs::find_filter_vec(const QString& vecname) if (qtemp.isNull()) { Vecs::assign_option(vec.name, &arg, arg.defaultvalue); } else { - Vecs::assign_option(vec.name, &arg, CSTR(qtemp)); + Vecs::assign_option(vec.name, &arg, qtemp); } } } @@ -233,7 +233,7 @@ Filter* FilterVecs::find_filter_vec(const QString& vecname) for (auto& arg : *args) { const QString opt = Vecs::get_option(options, arg.argstring); if (!opt.isNull()) { - Vecs::assign_option(vec.name, &arg, CSTR(opt)); + Vecs::assign_option(vec.name, &arg, opt); } } } @@ -308,7 +308,7 @@ void FilterVecs::disp_filter_vecs() const for (const auto& arg : *args) { if (!(arg.argtype & ARGTYPE_HIDDEN)) { printf(" %-18.18s %-.50s %s\n", - arg.argstring, arg.helpstring, + qPrintable(arg.argstring), qPrintable(arg.helpstring), (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); } } @@ -329,7 +329,7 @@ void FilterVecs::disp_filter_vec(const QString& vecname) const for (const auto& arg : *args) { if (!(arg.argtype & ARGTYPE_HIDDEN)) { printf(" %-18.18s %-.50s %s\n", - arg.argstring, arg.helpstring, + qPrintable(arg.argstring), qPrintable(arg.helpstring), (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); } } @@ -341,7 +341,7 @@ void FilterVecs::disp_help_url(const fl_vecs_t& vec, const arglist_t* arg) { printf("\t" WEB_DOC_DIR "/filter_%s.html", CSTR(vec.name)); if (arg) { - printf("#fmt_%s_o_%s", CSTR(vec.name), arg->argstring); + printf("#fmt_%s_o_%s", CSTR(vec.name), CSTR(arg->argstring)); } } @@ -355,12 +355,12 @@ void FilterVecs::disp_v1(const fl_vecs_t& vec) if (!(arg.argtype & ARGTYPE_HIDDEN)) { printf("option\t%s\t%s\t%s\t%s\t%s\t%s\t%s", CSTR(vec.name), - arg.argstring, - arg.helpstring, + CSTR(arg.argstring), + CSTR(arg.helpstring), Vecs::name_option(arg.argtype), - arg.defaultvalue ? arg.defaultvalue : "", - arg.minvalue ? arg.minvalue : "", - arg.maxvalue ? arg.maxvalue : ""); + CSTR(arg.defaultvalue), + CSTR(arg.minvalue), + CSTR(arg.maxvalue)); disp_help_url(vec, &arg); printf("\n"); } diff --git a/reference/filter1.txt b/reference/filter1.txt index 9520ab7d1..b2448767b 100644 --- a/reference/filter1.txt +++ b/reference/filter1.txt @@ -34,8 +34,8 @@ option track split Split by date or time interval (see README) string https:/ option track sdistance Split by distance string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_sdistance option track merge Merge multiple tracks for the same way string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_merge option track name Use only track(s) where title matches given name string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_name -option track start Use only track points after this timestamp integer https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_start -option track stop Use only track points before this timestamp integer https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_stop +option track start Use only track points after this timestamp string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_start +option track stop Use only track points before this timestamp string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_stop option track title Basic title for new track(s) string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_title option track fix Synthesize GPS fixes (PPS, DGPS, 3D, 2D, NONE) string https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_fix option track course Synthesize course boolean https://www.gpsbabel.org/WEB_DOC_DIR/filter_track.html#fmt_track_o_course diff --git a/trackfilter.h b/trackfilter.h index 3dba0b4b3..13d04669e 100644 --- a/trackfilter.h +++ b/trackfilter.h @@ -116,12 +116,12 @@ class TrackFilter:public Filter }, { TRACKFILTER_START_OPTION, &opt_start, - "Use only track points after this timestamp", nullptr, ARGTYPE_INT, + "Use only track points after this timestamp", nullptr, ARGTYPE_STRING, ARG_NOMINMAX, nullptr }, { TRACKFILTER_STOP_OPTION, &opt_stop, - "Use only track points before this timestamp", nullptr, ARGTYPE_INT, + "Use only track points before this timestamp", nullptr, ARGTYPE_STRING, ARG_NOMINMAX, nullptr }, { diff --git a/vecs.cc b/vecs.cc index ebad270ab..0df9ee641 100644 --- a/vecs.cc +++ b/vecs.cc @@ -22,6 +22,7 @@ #include "vecs.h" #include // for QByteArray +#include // for QChar #include // for QDebug #include // for QDir, QDir::Files, QDir::Name #include // for QFileInfo @@ -34,7 +35,6 @@ #include // for sort #include // for assert -#include // for isdigit #include // for printf, putchar, sscanf #include "defs.h" // for arglist_t, ff_vecs_t, ff_cap, fatal, CSTR, ARGTYPE_TYPEMASK, case_ignore_strcmp, global_options, global_opts, warning, xfree, ARGTYPE_BOOL, ff_cap_read, ff_cap_write, ARGTYPE_HIDDEN, ff_type_internal, xstrdup, ARGTYPE_INT, ARGTYPE_REQUIRED, ARGTYPE_FLOAT @@ -107,8 +107,7 @@ extern ff_vecs_t format_garmin_xt_vecs; #define MYNAME "vecs" -struct Vecs::Impl -{ +struct Vecs::Impl { /* * Having these LegacyFormat instances be non-static data members * prevents the static initialization order fiasco because @@ -639,9 +638,64 @@ void Vecs::init_vecs() style_list = create_style_vec(); } -int Vecs::is_integer(const char* c) +bool Vecs::is_integer(const QString& val) +{ +#if 1 + /* FIXME: Using scanf to validate input is not recommened. + * Users may have taken advantage of this flexibilty + * when interpreting ARGTYPE_INT. + * INT05-C. Do not use input functions to convert character + * data if they cannot handle all possible inputs + */ + // note sscanf doesn't do range checking + // note some users allow hex input. + // note some users may interpret trailing data after + // conversion, typically to denote a unit. + int test; + return 1 == sscanf(CSTR(val), "%d", &test); +#else + try { + (void) std::stoi(val.toStdString(), nullptr, 10); + } catch (const std::invalid_argument&) { + return false; + } catch (const std::out_of_range&) { + return false; + } + return true; +#endif +} + +bool Vecs::is_float(const QString& val) +{ +#if 1 + /* FIXME: Using scanf to validate input is not recommened. + * Users may have taken advantage of this flexibilty + * when interpreting ARGTYPE_FLOAT. + * INT05-C. Do not use input functions to convert character + * data if they cannot handle all possible inputs + */ + // note sscanf doesn't do range checking + // note some users may interpret trailing data after + // conversion, typically to denote a unit. + double test; + return 1 == sscanf(CSTR(val), "%lf", &test); +#else + try { + (void) std::stod(val.toStdString(), nullptr); + } catch (const std::invalid_argument&) { + return false; + } catch (const std::out_of_range&) { + return false; + } + return true; +#endif +} + +bool Vecs::is_bool(const QString& val) { - return isdigit(c[0]) || ((c[0] == '+' || c[0] == '-') && isdigit(c[1])); + return val.startsWith('y', Qt::CaseInsensitive) || + val.startsWith('n', Qt::CaseInsensitive) || + (!val.isEmpty() && val.at(0).isDigit()); } void Vecs::exit_vecs() @@ -663,12 +717,10 @@ void Vecs::exit_vecs() style_list.squeeze(); } -void Vecs::assign_option(const QString& module, arglist_t* arg, const char* val) +void Vecs::assign_option(const QString& module, arglist_t* arg, const QString& val) { - const char* c; - if (arg->argval == nullptr) { - fatal("%s: No local variable defined for option \"%s\"!", qPrintable(module), arg->argstring); + fatal("%s: No local variable defined for option \"%s\"!\n", qPrintable(module), qPrintable(arg->argstring)); } if (arg->argvalptr != nullptr) { @@ -679,65 +731,51 @@ void Vecs::assign_option(const QString& module, arglist_t* arg, const char* val) *arg->argval = nullptr; } - if (val == nullptr) { + if (val.isNull()) { return; } - // Fixme - this is probably somewhere between wrong and less than great. If you have an option "foo" - // and want to set it to the value "foo", this code will prevent that from happening, but we seem to have - // code all over the place that relies on this. :-/ - if (case_ignore_strcmp(val, arg->argstring) == 0) { - c = ""; - } else { - c = val; - } + QString rval(val); switch (arg->argtype & ARGTYPE_TYPEMASK) { case ARGTYPE_INT: - if (*c == '\0') { - c = "0"; + if (val.isEmpty()) { + rval = '0'; } else { - int test; - if (1 != sscanf(c, "%d", &test)) { - fatal("%s: Invalid parameter value %s for option %s", qPrintable(module), val, arg->argstring); + if (!is_integer(val)) { + fatal("%s: Invalid parameter value \"%s\" for option %s!\n", qPrintable(module), qPrintable(val), qPrintable(arg->argstring)); } } break; case ARGTYPE_FLOAT: - if (*c == '\0') { - c = "0"; + if (val.isEmpty()) { + rval = '0'; } else { - double test; - if (1 != sscanf(c, "%lf", &test)) { - fatal("%s: Invalid parameter value %s for option %s", qPrintable(module), val, arg->argstring); + if (!is_float(val)) { + fatal("%s: Invalid parameter value \"%s\" for option %s!\n", qPrintable(module), qPrintable(val), qPrintable(arg->argstring)); } } break; case ARGTYPE_BOOL: - if (*c == '\0') { - c = "1"; + if (val.isEmpty()) { + rval = '1'; } else { - switch (*c) { - case 'Y': - case 'y': - c = "1"; - break; - case 'N': - case 'n': - c = "0"; - break; - default: - if (isdigit(*c)) { - if (*c == '0') { - c = "0"; + if (val.startsWith('y', Qt::CaseInsensitive)) { + rval = '1'; + } else if (val.startsWith('n', Qt::CaseInsensitive)) { + rval = '0'; + } else { + // This works for decimal digits in the BMP (and thus represented by one QChar). + if (val.at(0).isDigit()) { + if (0 == val.at(0).digitValue()) { + rval = '0'; } else { - c = "1"; + rval = '1'; } } else { - warning(MYNAME ": Invalid logical value '%s' (%s)!\n", c, qPrintable(module)); - c = "0"; + warning("%s :Invalid logical value \"%s\" for option %s!\n", qPrintable(module), qPrintable(val), qPrintable(arg->argstring)); + rval = '0'; } - break; } } break; @@ -746,10 +784,10 @@ void Vecs::assign_option(const QString& module, arglist_t* arg, const char* val) /* for bool options without default: don't set argval if "FALSE" */ if (((arg->argtype & ARGTYPE_TYPEMASK) == ARGTYPE_BOOL) && - (*c == '0') && (arg->defaultvalue == nullptr)) { + rval.startsWith('0') && (arg->defaultvalue == nullptr)) { return; } - *arg->argval = arg->argvalptr = xstrdup(c); + *arg->argval = arg->argvalptr = xstrdup(rval); } void Vecs::disp_vec_options(const QString& vecname, const QVector* args) @@ -758,8 +796,8 @@ void Vecs::disp_vec_options(const QString& vecname, const QVector* ar for (const auto& arg : *args) { if (*arg.argval && arg.argval) { printf("options: module/option=value: %s/%s=\"%s\"", - qPrintable(vecname), arg.argstring, *arg.argval); - if (arg.defaultvalue && (case_ignore_strcmp(arg.defaultvalue, *arg.argval) == 0)) { + qPrintable(vecname), qPrintable(arg.argstring), *arg.argval); + if (case_ignore_strcmp(arg.defaultvalue, *arg.argval) == 0) { printf(" (=default)"); } printf("\n"); @@ -810,7 +848,7 @@ Format* Vecs::find_vec(const QString& vecname) if (!options.isEmpty()) { const QString opt = get_option(options, arg.argstring); if (!opt.isNull()) { - assign_option(vec.name, &arg, CSTR(opt)); + assign_option(vec.name, &arg, opt); continue; } } @@ -821,7 +859,7 @@ Format* Vecs::find_vec(const QString& vecname) if (qopt.isNull()) { assign_option(vec.name, &arg, arg.defaultvalue); } else { - assign_option(vec.name, &arg, CSTR(qopt)); + assign_option(vec.name, &arg, qopt); } } } @@ -863,7 +901,7 @@ Format* Vecs::find_vec(const QString& vecname) if (!options.isEmpty()) { const QString opt = get_option(options, arg.argstring); if (!opt.isNull()) { - assign_option(svec.name, &arg, CSTR(opt)); + assign_option(svec.name, &arg, opt); continue; } } @@ -874,7 +912,7 @@ Format* Vecs::find_vec(const QString& vecname) if (qopt.isNull()) { assign_option(svec.name, &arg, arg.defaultvalue); } else { - assign_option(svec.name, &arg, CSTR(qopt)); + assign_option(svec.name, &arg, qopt); } } } @@ -901,9 +939,9 @@ Format* Vecs::find_vec(const QString& vecname) * Find and return a specific argument in an arg list. * Modelled approximately after getenv. */ -QString Vecs::get_option(const QStringList& options, const char* argname) +QString Vecs::get_option(const QStringList& options, const QString& argname) { - QString rval; + QString rval; // null for (const auto& option : options) { int split = option.indexOf('='); @@ -919,7 +957,7 @@ QString Vecs::get_option(const QStringList& options, const char* argname) assert(!rval.isNull()); break; } else { - rval = option_name; // not null, possibly empty. + rval = ""; // not null, empty assert(!rval.isNull()); break; } @@ -1174,9 +1212,9 @@ void Vecs::disp_v3(const vecinfo_t& vec) CSTR(arg.argstring), CSTR(arg.helpstring), name_option(arg.argtype), - arg.defaultvalue.isEmpty() ? "" : CSTR(arg.defaultvalue), - arg.minvalue.isEmpty() ? "" : CSTR(arg.minvalue), - arg.maxvalue.isEmpty() ? "" : CSTR(arg.maxvalue)); + CSTR(arg.defaultvalue), + CSTR(arg.minvalue), + CSTR(arg.maxvalue)); } disp_help_url(vec, arg.argstring); printf("\n"); @@ -1246,21 +1284,44 @@ bool Vecs::validate_args(const QString& name, const QVector* args) #endif for (const auto& arg : *args) { if ((arg.argtype & ARGTYPE_TYPEMASK) == ARGTYPE_INT) { - if (arg.defaultvalue && - ! is_integer(arg.defaultvalue)) { + if (!arg.defaultvalue.isNull() && !is_integer(arg.defaultvalue)) { Warning() << name << "Int option" << arg.argstring << "default value" << arg.defaultvalue << "is not an integer."; ok = false; } - if (arg.minvalue && - ! is_integer(arg.minvalue)) { + if (!arg.minvalue.isNull() && !is_integer(arg.minvalue)) { Warning() << name << "Int option" << arg.argstring << "minimum value" << arg.minvalue << "is not an integer."; ok = false; } - if (arg.maxvalue && - ! is_integer(arg.maxvalue)) { + if (!arg.maxvalue.isNull() && !is_integer(arg.maxvalue)) { Warning() << name << "Int option" << arg.argstring << "maximum value" << arg.maxvalue << "is not an integer."; ok = false; } + } else if ((arg.argtype & ARGTYPE_TYPEMASK) == ARGTYPE_FLOAT) { + if (!arg.defaultvalue.isNull() && !is_float(arg.defaultvalue)) { + Warning() << name << "Float option" << arg.argstring << "default value" << arg.defaultvalue << "is not an float."; + ok = false; + } + if (!arg.minvalue.isNull() && !is_float(arg.minvalue)) { + Warning() << name << "Float option" << arg.argstring << "minimum value" << arg.minvalue << "is not an float."; + ok = false; + } + if (!arg.maxvalue.isNull() && !is_float(arg.maxvalue)) { + Warning() << name << "Float option" << arg.argstring << "maximum value" << arg.maxvalue << "is not an float."; + ok = false; + } + } else if ((arg.argtype & ARGTYPE_TYPEMASK) == ARGTYPE_BOOL) { + if (!arg.defaultvalue.isNull() && !is_bool(arg.defaultvalue)) { + Warning() << name << "Bool option" << arg.argstring << "default value" << arg.defaultvalue << "is not an bool."; + ok = false; + } + if (!arg.minvalue.isNull() && !is_bool(arg.minvalue)) { + Warning() << name << "Bool option" << arg.argstring << "minimum value" << arg.minvalue << "is not an bool."; + ok = false; + } + if (!arg.maxvalue.isNull() && !is_bool(arg.maxvalue)) { + Warning() << name << "Bool option" << arg.argstring << "maximum value" << arg.maxvalue << "is not an bool."; + ok = false; + } } } } diff --git a/vecs.h b/vecs.h index 55b908ecb..b0f9ebe90 100644 --- a/vecs.h +++ b/vecs.h @@ -47,10 +47,10 @@ class Vecs void init_vecs(); void exit_vecs(); - static void assign_option(const QString& module, arglist_t* arg, const char* val); + static void assign_option(const QString& module, arglist_t* arg, const QString& val); static void disp_vec_options(const QString& vecname, const QVector* args); static void validate_options(const QStringList& options, const QVector* args, const QString& name); - static QString get_option(const QStringList& options, const char* argname); + static QString get_option(const QStringList& options, const QString& argname); Format* find_vec(const QString& vecname); void disp_vecs() const; void disp_vec(const QString& vecname) const; @@ -113,7 +113,9 @@ class Vecs /* Member Functions */ - static int is_integer(const char* c); + static bool is_integer(const QString& val); + static bool is_float(const QString& val); + static bool is_bool(const QString& val); static QVector create_style_vec(); QVector sort_and_unify_vecs() const; static void disp_v1(ff_type t);