Checking for duplicate list names

[yakra]

C++:
Building the list_names set & checking for membership is redundant. When creating route_hash, we can use the return value of unordered_map::insert to determine whether each .list name already exists.

Python:
I'll have to refamiliarize myself with sets & dicts. There may not be the same opportunities for improving efficiency.

Both:
We check for duplicate primary .list names, but not AltRouteNames. Something to be fixed in tandem.

[yakra]

Both languages:
Duplicate checks are case-sensitive and need to be case-insensitive.

[yakra]

Work on this has uncovered a well-hidden C++ bug. Routes that have no AltRouteNames specified in the .csv get one AltRouteName consisting of an empty string.
Starting at:

DataProcessing/siteupdate/cplusplus/classes/Route/Route.cpp

Line 55 in 8147a1f

// alt_route_names

size_t l = 0;
for (size_t r = 0; r != -1; l = r+1)
{	r = arn_str.find(',', l);
	// For routes with a blank AltRouteNames field, there's no comma,
	// and r gets set to -1.
	alt_route_names.emplace_back(arn_str, l, r-l);
	// This forwards (arn_str, l, r-l) to the std::string substring constructor.
	// http://www.cplusplus.com/reference/string/string/string/
	// l is still 0, r-l is -1, and we copy until the end of arn_str...
	// ...which is also its beginning.
	// Thus an empty substring is put into the alt_route_names container.
}

Creating ConnectedRoute root lists is handled similarly, but symptoms haven't shown up in practice, because every ConnectedRoute should have at least one route.

• Including a _con.csv line with a blank roots field should produce a harmless extraneous Could not find Route matching ConnectedRoute root in system before the expected "No roots in whatever_con.csv line:
• Gotta redo this. Again. I've always had difficulty with this part of the code, hence just unrolling the loops prior to error checks for field lengths #305.

  // alt_route_names
  size_t l = 0;
  for (size_t r = arn_str.find(','); r != -1; r = arn_str.find(',', l))
  { alt_route_names.emplace_back(arn_str, l, r-l);
   l = r+1;
  }

  $DEITY I hope this works...
  LOLNOPE!
  Now we're dropping the last AltRouteName from routes that have them.

[yakra]

Progress. C++ fix looks functional so far.
I've confirmed several cases of broken maps/stats due to duplicate AltRouteNames:

• bickendan.list, okroads.list & ovoss_old.list all refer to I-49Tex rather than the Bella Vista bypass.
  @ovoss's current file no longer uses this AltRouteName.
  @Bickendan & okroads could fix by changing to the current canonical name.
• ovoss_old.list & Ugnaught2.list both contain MT US212_W.
• Right now nobody is credited with any mileage on IRL N60, but @bejacob should have some travels there. His stats won't be right until we fix this on our end.

[yakra]

[85.8] ABORTING due to 9 errors:
1: Duplicate main list name in deuni.b006: 'DEU-NI B6' already points to deuni.a369
2: Duplicate main list name in deust.b006: 'DEU-ST B6' already points to deust.a036
3: Duplicate alt route name in irl.r380: 'IRL N60' already points to irl.n060
4: Duplicate alt route name in tur.o005gor: 'TUR O5gor' already points to tur.o005gor
5: Duplicate alt route name in tur.o005akh: 'TUR O5akh' already points to tur.o005akh
6: Duplicate alt route name in mt.nentrd: 'MT US212_W' already points to mt.us212coo
7: Duplicate main list name in nv.nv430: 'NV NV430' already points to nv.us395altwas
8: Duplicate alt route name in nor.fv220: 'NOR Fv22' already points to nor.fv022
9: Duplicate main list name in ar.ar549: 'AR AR549' already points to ar.i049tex

[yakra]

Work on this has uncovered a well-hidden C++ bug. Routes that have no AltRouteNames specified in the .csv get one AltRouteName consisting of an empty string.

Turns out siteupdate.py had an identical bug. So to that end, the C++ snippet I posted was successful in that it faithfully replicated Python's str.split functionality.
A quick check in the Route constructor solves this:

if len(fields[7]) == 0:
 self.alt_route_names = []
else:
 self.alt_route_names = fields[7].split(",")

This way, we get the desired empty list rather than a list of one empty string.

This keeps a bunch of extraneous malformed region + space + nothing keys that would never match any lookups produced by .list file processing out of route_hash. In practice though, I doubt this that would make .list processing any noticeable amount faster.

---

Python version is done, with no diffs from the C++ version.

Cleaned up the reporting of unmatched roots & duplicate list names -- a lot of the code here looks like vestiges of the early days when these were handled as datacheck errors before ErrorList was implemented. The counts of numbers of errors added are less important now, as we can just see the errors themselves listed when siteupdate aborts, so I cut them out to simplify the code.
The list_names and duplicate_list_names sets are gone now.
Also removed an old unused commented-out block -- was this used for matching datacheck FP entries when first porting them over from CHM?

I'll be pushing to GitHub shortly, though in order to keep site updates going rather than aborting due to errors, I'll hold off on a pull request until the errors are fixed on the HighwayData side.
...unless @jteresco wants to give the C++ version a try for a site update. ;)
It will still produce a valid .sql file before aborting. There will just be some differences in how the AltRouteName collisions are handled in .list processing, and thus stats & maps.

• Old: route_hash key = the last Route processed
• New: route_hash key = the 1st Route processed

[yakra]

Three's still potential for refactoring & simplifying, with minor efficiency improvements in the process.

C++, since region-splitting support was added, has a con_route pointer in each Route object. This makes it a snap when assigning these pointers (in the ConnectedRoute ctor) to check whether one already exists. If so, then el.add_error.
This allows us to dispense with the con_roots set altogether (an idea that'd enable us to get rid of roots is at #189 (comment)).
We'd still have to iterate through Routes after processing ConnectedRoutes, and look for those with a null con_route; we can add this into the same loop used for Creating route hash table and checking for duplicate list names & roots.

We'd lose the Check FAILED: FOO routes != BAR connected route roots error, but no big deal; that's only triggered when there are underlying errors; the actual things we'd need to fix.

Python's type system makes this a less clear winner. What's less expensive, manipulating sets, or adding a new variable to an object that may or may not have a given propery? Probably the latter?

• To start out, try replacing the set contents with Route objects instead of strings. Hashing (I assume) memory locations should be faster than hashing strings (which is already plenty fast).
  This will allow us to easily have more helpful error reports that list what ConnectedRoute already contains a given root.
  Wait... We need strings in roots because we want to find multiple Route objects with the same root string. This means making a root_hash, which is useful for other things too.

Both: list systems:

• Duplicate root in route lists: aln.gp005
  ->
  Duplicate root in alngt.csv: aln.gp005 already in alngp.csv
  Requires root_hash. What the hey; I'll implement it; it'll be useful.

• route aln.gp005 not matched by any connected route root.
  ->
  alngp.csv: root aln.gp005 not matched by any connected route root.
  WIP removed the 2nd pass iterating thru highway_systems & route_lists, meaning we no longer get multiple reports if a given root is both duplicated and unmatched in connected routes.
  The C++ solution above takes care of this; it's starting to make sense to switch Python over too.

• Duplicate root in con_route lists: aln.gp005
  ->
  Duplicate root in alngt_con.csv: aln.gp005 already in alngp_con.csv
  Again, storing a con_route will help.

[yakra]

I think I saw a speedup in Python Processing traveler list files. This makes sense because having removed the bug where every AltRouteName-less Route gets one blank AltRouteName, we're no longer comparing this blank string twice for every list file line.

Have another look to be sure:
Nope, I'm full of hooey. But still optimistic about a potential efficiency boost.