fix mkshort unique (#1168)

* fix mkshort unique.

1. we were off by one when deciding if we could just concatentate
the generated suffix to the name, failing to use the last available
character.
2. when attempting to copy the generated suffix to a presumably
shortened name, and the generated suffix was longer than the name,
the target of the copy was outside the name buffer.  In the common
case that the rank of size_t was greater than the rank of int, the
target was well beyond the end of name, not before as one would expect
with signed arithmetic.

With the new algorithm when the target length is insufficient to
fit both the name and the suffix we will only truncate the name as
required to fit the truncated name and the complete suffix in the
target length(as opposed to the original length of the name).
We fatal if this is not possible.

Add a test case to exercise the make unique code.

* fix reference mode.

* add notes for future enhancements

* fix testcase cut and paste booboo

* refactor mkshort input from char* to QByteArray.

defs.h

@@ -854,7 +854,7 @@ using ff_readposn = Waypoint* (*)(posn_status*);
 struct mkshort_handle_imp; // forward declare, definition in mkshort.cc
 using short_handle = mkshort_handle_imp*;
 
-QByteArray mkshort(short_handle, const char*, bool);
+QByteArray mkshort(short_handle, const QByteArray&, bool);
 QString mkshort(short_handle, const QString&);
 short_handle mkshort_new_handle();
 QString mkshort_from_wpt(short_handle h, const Waypoint* wpt);

garmin.cc

@@ -890,8 +890,8 @@ waypoint_prepare()
      */
     QByteArray ident = mkshort(mkshort_handle,
                                global_opts.synthesize_shortnames ?
-                               str_from_unicode(src).constData() :
-                               str_from_unicode(wpt->shortname).constData(),
+                               str_from_unicode(src) :
+                               str_from_unicode(wpt->shortname),
                                false);
     /* Should not be a strcpy as 'ident' isn't really a C string,
      * but rather a garmin "fixed length" buffer that's padded

mkshort.cc

@@ -19,8 +19,9 @@
 
  */
 
+#include <cassert>     // for assert
 #include <cctype>      // for isspace, toupper, isdigit
-#include <cstdio>      // for snprintf, size_t
+#include <climits>     // for INT_MAX
 #include <cstring>     // for strlen, memmove, strchr, strcpy, strncmp, strcat, strncpy
 
 #include <QByteArray>  // for QByteArray
@@ -49,7 +50,7 @@ class ShortNameKey;
 using ShortNameHash = QHash<ShortNameKey, uniq_shortname*>;
 class ShortNameKey {
 public:
-  ShortNameKey(char* name) : shortname(name) {} /* converting constructor */
+  ShortNameKey(const QByteArray& name) : shortname(name) {} /* converting constructor */
 
   using namehash_size_type = ShortNameHash::size_type;
   friend namehash_size_type qHash(const ShortNameKey &key, namehash_size_type seed = 0) noexcept
@@ -110,7 +111,7 @@ mkshort_new_handle()
 
 static
 uniq_shortname*
-is_unique(mkshort_handle_imp* h, char* name)
+is_unique(mkshort_handle_imp* h, const QByteArray& name)
 {
   if (h->namelist.contains(name)) {
     return h->namelist.value(name);
@@ -120,34 +121,38 @@ is_unique(mkshort_handle_imp* h, char* name)
 
 static
 void
-add_to_hashlist(mkshort_handle_imp* h, char* name)
+add_to_hashlist(mkshort_handle_imp* h, const QByteArray& name)
 {
   h->namelist.insert(name, new uniq_shortname);
 }
 
-char*
-mkshort_add_to_list(mkshort_handle_imp* h, char* name)
+static
+void
+mkshort_add_to_list(mkshort_handle_imp* h, QByteArray& name)
 {
-  uniq_shortname* s;
+  static_assert(!std::is_signed<decltype(h->target_len)>::value,
+                "simplify the following logic if target length is signed.");
+  assert(h->target_len <= INT_MAX);
+  int target_len = h->target_len;
 
+  uniq_shortname* s;
   while ((s = is_unique(h, name))) {
-    char tbuf[13];
-    size_t l = strlen(name);
 
-    s->conflictctr++;
+    QByteArray suffix(".");
+    suffix.append(QByteArray::number(++s->conflictctr));
 
-    int dl = snprintf(tbuf, sizeof(tbuf), ".%d", s->conflictctr);
-
-    if (l + dl < h->target_len) {
-      name = (char*) xrealloc(name, l + dl + 1);
-      strcat(name, tbuf);
+    if (name.size() + suffix.size() <= target_len) {
+      name.append(suffix);
+    } else if (suffix.size() <= target_len) {
+      // FIXME: utf8 => grapheme truncate
+      name.truncate(target_len - suffix.size());
+      name.append(suffix);
     } else {
-      strcpy(&name[l-dl], tbuf);
+      fatal("mkshort failure, the specified short length is insufficient.\n");
     }
   }
 
   add_to_hashlist(h, name);
-  return name;
 }
 
 void
@@ -250,7 +255,9 @@ void
 setshort_length(short_handle h, int l)
 {
   auto* hdl = (mkshort_handle_imp*) h;
-  if (l == 0) {
+  if (l < 0) {
+    fatal("mkshort: short length must be non-negative.\n");
+  } else if (l == 0) {
     hdl->target_len = default_target_len;
   } else {
     hdl->target_len = l;
@@ -355,7 +362,7 @@ setshort_mustuniq(short_handle h, int i)
 }
 
 QByteArray
-mkshort(short_handle h, const char* istring, bool is_utf8)
+mkshort(short_handle h, const QByteArray& istring, bool is_utf8)
 {
   char* ostring;
   char* tstring;
@@ -371,7 +378,7 @@ mkshort(short_handle h, const char* istring, bool is_utf8)
     result.remove(QChar::ReplacementCharacter);
     ostring = xstrdup(result.toUtf8().constData());
   } else {
-    ostring = xstrdup(istring);
+    ostring = xstrdup(istring.constData());
   }
 
   /*
@@ -556,18 +563,18 @@ mkshort(short_handle h, const char* istring, bool is_utf8)
     ostring = xstrdup(hdl->defname);
   }
 
-  if (hdl->must_uniq) {
-    ostring = mkshort_add_to_list(hdl, ostring);
-  }
   QByteArray rval(ostring);
   xfree(ostring);
+  if (hdl->must_uniq) {
+    mkshort_add_to_list(hdl, rval);
+  }
   return rval;
 }
 
 QString
 mkshort(short_handle h, const QString& istring)
 {
-  return mkshort(h, CSTR(istring), true);
+  return mkshort(h, istring.toUtf8(), true);
 }
 
 /*

reference/mkshort.style

@@ -6,12 +6,12 @@ SHORTLEN	4
 # FILE LAYOUT DEFINITIIONS:
 #
 FIELD_ENCLOSER    DOUBLEQUOTE
-FIELD_DELIMITER		COMMA
+FIELD_DELIMITER		COMMASPACE
 RECORD_DELIMITER	NEWLINE
 BADCHARS		COMMA
 #
 # INDIVIDUAL DATA FIELDS, IN ORDER OF APPEARANCE:
 #
-IFIELD  LAT_DECIMAL, "", "%.5f"
-IFIELD  LON_DECIMAL, "", "%.5f"
+IFIELD  LAT_DECIMAL, "", "%8.5f"
+IFIELD  LON_DECIMAL, "", "%8.5f"
 IFIELD	SHORTNAME, "", "%s"