Provided callback `sortMod` does not provide a strict weak ordering

[leandor]

During my debug sessions these last days, I compiled vs the debug version of Boost and the STL, and got an assertion while trying to build the Bashed Patch that pointed to the current sorting callback not being correct.

I'm referring to this function:

Original version of sortMod()

See [here](https://github.com/wrye-bash/CBash/blob/1f90085a27c6847685d6eb9589a1d7fda5c85862/src/Collection.cpp#L102) ``` c++ bool sortMod(ModFile *lhs, ModFile *rhs) { //Esp's sort after esm's //Non-existent esms sort before existing esps //Non-existent esms retain their relative load order //Existing esps sort by modified date //Non-existent esps sort before existing esps //Non-existent esps retain their relative load order //New esps load last #ifndef LHS_BEFORE_RHS #define LHS_BEFORE_RHS true #define LHS_AFTER_RHS false #endif if(lhs->TES4.IsESM()) { if(rhs->TES4.IsESM()) { if(lhs->ModTime == 0) { if(rhs->ModTime == 0) return LHS_BEFORE_RHS; return LHS_AFTER_RHS; } if(rhs->ModTime == 0) return LHS_BEFORE_RHS; return lhs->ModTime < rhs->ModTime; } return LHS_BEFORE_RHS; } if(rhs->TES4.IsESM()) return LHS_AFTER_RHS; if(lhs->Flags.IsCreateNew) { if(rhs->Flags.IsCreateNew) return LHS_BEFORE_RHS; return LHS_AFTER_RHS; } if(rhs->Flags.IsCreateNew) return LHS_BEFORE_RHS; if(lhs->ModTime == 0) return LHS_BEFORE_RHS; if(rhs->ModTime == 0) return LHS_AFTER_RHS; return lhs->ModTime < rhs->ModTime; } ```

The idea seems to be, according to the rules that are spelled out on the header, to break the set into two partitions, ESMs|ESPs, and within those partitions each file sorts according to mod time, but taking into account that new files (whose mod time is 0, or that has the CreatedNew flag set) need to sort last.

So, I simplified it to be more clear and to remove the assertion. It became this:

New modified version.

``` c++ bool sortMod(ModFile *lhs, ModFile *rhs) { //Esp's sort after esm's //Non-existent esms sort before existing esps //Non-existent esms retain their relative load order //Existing esps sort by modified date //Non-existent esps sort before existing esps //Non-existent esps retain their relative load order //New esps load last #ifndef LHS_BEFORE_RHS #define LHS_BEFORE_RHS true #define LHS_AFTER_RHS false #endif const time_t MIN_TIME = std::numeric_limits::min(); const time_t MAX_TIME = std::numeric_limits::max(); bool leftEsm = lhs->TES4.IsESM(); bool rightEsm = rhs->TES4.IsESM(); bool leftNew = (lhs->ModTime == 0) || lhs->Flags.IsCreateNew; bool rightNew = (rhs->ModTime == 0) || rhs->Flags.IsCreateNew; time_t leftTime = leftNew ? MAX_TIME : lhs->ModTime; time_t rightTime = rightNew ? MAX_TIME : rhs->ModTime; if (leftEsm != rightEsm) { return leftEsm ? LHS_BEFORE_RHS : LHS_AFTER_RHS; } else if (leftNew != rightNew) { return leftNew ? LHS_AFTER_RHS : LHS_BEFORE_RHS; } else // They are in the same "partition", strict ordering applies within each { return leftTime < rightTime; } // should never reach here, but not important enough to assert IMO. return LHS_BEFORE_RHS; } ```

It seems to be 'doing the right thing' ^:tm:, but I haven't tested extensively yet. I've built a full Bashed Patch after the change, though, and the assertion was gone.

I'll probably gonna create the first test for checking this function and verify the ordering :)

I hope I'm not missing something obvious :fearful:

[Infernio]

Closed in 4d1b5614f6120b7845bb43d7decb2f8ea3a8f4cd.