Respect clue list ordering from file and expand initial clue selection.

[jpd236]

Change Clues from a map, which is ordered alphabetically, to a vector,
which retains insertion order. This should make no difference for
regular crosswords (and there is special logic to handle Across/Down
even if out of order), but retains the order from variety puzzle formats
which is more likely to be the intended viewing order.

Also expand the logic used to select the initial clue; previously, we
only selected the first clue's first square if there was one clue list.
We now do this even if there is more than one clue list. This is more
likely to be reasonable now that clue list ordering is maintained from
the original file.

Fixes #134

[mrichards42]

Makes sense! I'll give this a quick test this week.

[mrichards42]

I managed to trigger a segfault -- It doesn't happen all the time, but I can hit it pretty consistently by loading a puz file from the downloader. I'm not sure why it only seems to trigger there, but it looks like it's coming from these lines in Puzzle::SetAllClues (which is only used in load_puz):

xword/puz/Puzzle.cpp

Lines 185 to 186 in 133af2b

	ClueList & across = SetClueList(puzT("Across"), ClueList());
	ClueList & down = SetClueList(puzT("Down"), ClueList());

I'm pretty sure what's happening is that the second SetClueList causes the vector to reallocate, which invalidates the across reference. With m_clues.reserve(2) above those two lines it seems to work for me. std::map shouldn't invalidate references on inserts, so we wouldn't have run into this before. I did a quick glance around the code to see if this pattern (acquire a reference/pointer, add a new clue list) is used anywhere else and didn't find anything obvious, but it's definitely possible I'm missing something. Fortunately in the lua code we should be immediately converting from ClueList to a table, so wouldn't expect this bug to crop in there.

[jpd236]

That makes sense to me - thanks for tracking this down! I don't generally use the downloader so I hadn't noticed.

Calling reserve(2) feels a bit hacky to me since it's relying on an internal implementation detail of the clue list (the fact that we expose the underlying type is IMHO not ideal - thankfully we didn't rely too much on implementation details of Map outside of the Clues class, or swapping to a vector would have been even harder). I also considered calling SetClueList twice, then calling GetClueList afterwards and taking the references from there, since they'd be valid without any insertions happening in the middle, but that also felt a bit clunky to me. So what I went with was initializing the ClueLists in local variables, and then setting them at the end once fully constructed.

I'm a little shaky on whether this is safe, though - I think that what happens is that the ClueList is copied when it's set, which means it's fine that the local variables go out of scope once Puzzle::SetAllClues returns. At least it isn't crashing :) But I wasn't 100% certain if this is a reasonable assumption here.

I also checked for other usage of SetClueList; the only slightly questionable one I was was in load_txt.cpp, but in that case it looks like we never modify across after the reference becomes invalidated.

[mrichards42]

I'm a little shaky on whether this is safe, though - I think that what happens is that the ClueList is copied when it's set, which means it's fine that the local variables go out of scope once Puzzle::SetAllClues returns. At least it isn't crashing :) But I wasn't 100% certain if this is a reasonable assumption here.

Yeah, the below code is pretty rough, but since operator[] returns ClueList &, line 150 is copying the cluelist arg, no pointers or anything happening under the hood.

xword/puz/Clue.hpp

Lines 148 to 150 in e310d2b

	ClueList & SetClueList(const string_t & direction, const ClueList & cluelist)
	{
	operator[](direction) = cluelist;

Segfault fixed, and I like your solution better than mine.