Fix error: control may reach end of non-void lambda

[GregoryLundberg]

If by_name and by_relation are both omitted, behave as if both were given: sort first by relation then by name.

[CelticMinstrel]

Hmm, while this does logically fix the error, I wonder if perhaps the proper approach in that case would be "don't sort at all"...

[GregoryLundberg]

Your call. I considered throwing, too.

[GregoryLundberg]

OK. Reverted to original behavior, before @Vultraz broke it in 4c9eab7.

Now, it returns true if neither is specified. That probably means "don't sort"

[CelticMinstrel]

Does returning true not assert on MSVC debug builds?

[GregoryLundberg]

Neither Release nor Debug for MSVC issued a message for falling through a lambda. Current this week; don't know if it's a regression from older versions.

[CelticMinstrel]

I was referring to the fact that a comparator always returning true is not a "proper" comparator which might cause MSVC debug libs to assert when it discovers that a<b is the same as b<a.

[Ja-MiT]

Returning true does not mean "don't sort". It means that given any two items, in any order, they are in the desired order. Depending on the implementation of std::sort, this might have the net effect of not sorting. However, the standard permits a net effect of shuffling things into a new order that would look no more sorted than the initial order. (Prior to C++11, this shuffling could have been O(n^2), a.k.a. the worst case for quicksort.)

To remove this dependence on an implementation detail, and to make the code a bit cleaner/clearer, it might be a good idea to stick if ( by_name || by_relation ) before the call to std::sort.

In addition, if a default case is still desired, it really should return false instead of true so that the comparison is a strict weak ordering, as required by std::sort.

[CelticMinstrel]

In addition, if a default case is still desired, it really should return false instead of true so that the comparison is a strict weak ordering, as required by std::sort.

This is precisely what I was worried about, as the MSVC debug libraries assert if it's not.

[Ja-MiT]

Hmm... would the lack of asserts being triggered reflect a lack of testing, or would it reflect difficulty hitting the problematic case? The return true is newish, but the possibility of reaching the lambda's end has been around for a while -- wouldn't that have triggered some sort of run-time error? If the problematic case is rare/impossible, maybe it's not worth worrying that much about re-ordering the list into another seemingly-random order? So maybe don't stick in the if I had suggested earlier, as it would make the code more complex without gain?

On a related note, the commit that was mentioned earlier (4c9eab7) removed a comment asking if the current approach was the simplest way to represent the sorting method. I would answer that with a "no" after coming up with the following:

	std::sort(users_sorted_.begin(), users_sorted_.end(), [&](const user_info* u1, const user_info* u2) {
		// Check each requested sort. If equivalent by that sort, fall down to the next sort.

		if ( by_relation ) {
			if ( u1->relation != u2->relation )
				return u1->relation < u2->relation;
		}

		if ( by_name ) {
			int icomp = translation::icompare(u1->name, u2->name);
			if ( icomp != 0 )
				return icomp < 0;
		}

		// All sorts compared equivalent, so u1 is not less than u2.
		return false;
	});

I did not test this, nor even check to see if it compiles, my apologies. (At some point the Wesnoth code base starting requiring package updates that are not readily available for my flavor of Linux, and I have not taken the time to update them manually.) To my eye, it looks like this should work, and it avoids a bit of code duplication.

[CelticMinstrel]

Knowing @GregoryLundberg I'd be more inclined to believe difficulty hitting a problematic case over lack of testing.

[GregoryLundberg]

return (by_name ? u1->name < u2->name : true) && (by_relation ? u1->relation < u2->relation : true);

I have problems with the original logic. It worked but it sure fails the sniff-test as can be seen by the amount of talk a one-liner has generated.

As I said, I would have preferred to throw but the original code returned so I went with that.

[Ja-MiT]

@GregoryLundberg Why would you prefer throwing?

[Vultraz]

For the record, I’m th one who originally wrot that code in the first place (including the comment about how succinct it was) and really, the only thing I was trying to do was get it as close to a 1-line return statement as possible. I’m also not even sure if what I wrote correctly translated the original logic that was there (it made use of sorting helper structs). Someone would need to check the log for that file.

[Vultraz]

It’s very possible I screwed something up since I didn’t (and still don’t) fully understand the intricacies of the sort function.

[Ja-MiT]

@Vultraz It looks like the original lambda reflected the helper struct logic except on two points. First, the lambda no longer casts an enum (the relation) to an int before comparing with <. (I think that's fine and simpler, so I'm just mentioning it for completeness.) Second, the lambda version ignored the case being discussed here, namely when all parameters are false. (Before the lambda, std::sort would not be called; after the lambda, std::sort gets called with a comparison that does not define a strict weak ordering.) So, a mostly good translation.

I would not put so much value on having a 1-line return statement. There is value in being succinct, but squashing a lot of logic into a single expression does not necessarily result in more optimized executable code. More likely, it results in source code that is harder for humans to read and maintain. For example, the line in question was
return (by_name ? u1->name < u2->name : true) && (by_relation ? u1->relation < u2->relation : true);
As part of this discussion, those trues have been seen as the value to return when both by_name and by_relation are false. However, it looks to me like those trues were probably intended to produce something like true && A, which evaluates to A. That is, they were intended as logical artifacts, not returnable values.

As another example, it looks like this attempt to have a 1-line return statement actually resulted in more complex code. I can see how the line in question might have originated as an attempt to handle all cases in a 1-line return statement. Then you realized that it would not handle the by_name && by_relation case correctly, so that case was broken out to be handled separately. But what about the remaining cases? If you assume that exactly one parameter is true, the statement could be expressed more simply as
return (by_name && u1->name < u2->name) || (by_relation && u1->relation < u2->relation);
In fact, I do not see why someone would go for the more complex version unless they were trying to handle the case where multiple parameters are true. (As a bonus, this version would also work when no parameters are true.)

[Vultraz]

That would be optimal, except it is possible for both flags to be true since this is controlled by two toggle buttons in the MP Lobby UI. However, I’ve been considering simply making sorted-by-name implicit and only leaving the sort-by-relation option, so that might render all of this invalid.

[Ja-MiT]

Right, but the exception is irrelevant. Putting that line in context:

if(by_name && by_relation) {
	return u1->relation < u2->relation || (u1->relation == u2->relation && u1->name < u2->name);
}
return (by_name && u1->name < u2->name) || (by_relation && u1->relation < u2->relation);

Once you broke out the earlier case, the remaining cases could be handled in a simpler manner, but you failed to see that, presumably because you were still hung up on having a 1-line return statement.

[Ja-MiT]

Minor point: "render all of this moot", not "invalid".

[Vultraz]

Committed that in 29e7e57