Sort definitions by priority before returning them.
Details
Details
Diff Detail
Diff Detail
- Repository
- R216 Syntax Highlighting
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Yes, this is nice, nice trick with value(0) to avoid adding an element to the vector but just letting the API default construct it.
Comment Actions
I think one error is the sort of the empty vector with the begin() + 1
But even then, make test fails, after fixing this.
Please take a look and run the tests.
Thanks
Comment Actions
I think the + 1 is the only real issue, the other issue stems from my local setting that I have no compiled in QRC.
Comment Actions
Actually, do we need a partial sort? Should we not just stable_sort the complete vector by prio? That should keep stuff with same prio untouched, or?
Comment Actions
Ah sorry completely missed that part. Unfortunately I'm out of office already, I'll take a look as soon as I'm back tomorrow.
| src/lib/repository.cpp | ||
|---|---|---|
| 83 | Isn't std::stable_sort what we want here? | |
Comment Actions
Thanks for taking a look again ;=)
I would propose to just stable_sort the vector, that should always yield the same result.
And I think the sorting is the only issue, the other problem I got did only occur with non-QRC syntax files and was introduced by myself ;=)
Comment Actions
Yeah, I just missed to recheck the sorting logic. Before the change we just needed the best match so the partial sort was completely sufficient. Now we need a complete sorted vector, so stable sort is the way to go ;)
Comment Actions
Cool, thanks! ...which now leads us to the point where the only missing part is a unit test for the two new public functions :-)