Use std::strong_ordering for version comparison

The actual motive here is the avoidance of integer overflow if we were
to make these use checked NixInts and retain the subtraction.

However, the actual *intent* of this code is a three-way comparison,
which can be done with operator<=>, so we should just do *that* instead.

Change-Id: I7f9a7da1f3176424b528af6d1b4f1591e4ab26bf
This commit is contained in:
Jade Lovelace 2024-07-12 14:56:30 +02:00 committed by Jade Lovelace
parent ee86e7f361
commit dd75711895
4 changed files with 18 additions and 17 deletions

View file

@ -4446,7 +4446,8 @@ static void prim_compareVersions(EvalState & state, const PosIdx pos, Value * *
{
auto version1 = state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.compareVersions");
auto version2 = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.compareVersions");
v.mkInt(compareVersions(version1, version2));
auto result = compareVersions(version1, version2);
v.mkInt(result < 0 ? -1 : result > 0 ? 1 : 0);
}
static RegisterPrimOp primop_compareVersions({

View file

@ -94,7 +94,7 @@ static bool componentsLT(const std::string_view c1, const std::string_view c2)
}
int compareVersions(const std::string_view v1, const std::string_view v2)
std::strong_ordering compareVersions(const std::string_view v1, const std::string_view v2)
{
auto p1 = v1.begin();
auto p2 = v2.begin();
@ -102,11 +102,11 @@ int compareVersions(const std::string_view v1, const std::string_view v2)
while (p1 != v1.end() || p2 != v2.end()) {
auto c1 = nextComponent(p1, v1.end());
auto c2 = nextComponent(p2, v2.end());
if (componentsLT(c1, c2)) return -1;
else if (componentsLT(c2, c1)) return 1;
if (componentsLT(c1, c2)) return std::strong_ordering::less;
else if (componentsLT(c2, c1)) return std::strong_ordering::greater;
}
return 0;
return std::strong_ordering::equal;
}

View file

@ -30,7 +30,7 @@ typedef std::list<DrvName> DrvNames;
std::string_view nextComponent(std::string_view::const_iterator & p,
const std::string_view::const_iterator end);
int compareVersions(const std::string_view v1, const std::string_view v2);
std::strong_ordering compareVersions(const std::string_view v1, const std::string_view v2);
DrvNames drvNamesFromArgs(const Strings & opArgs);
}

View file

@ -204,15 +204,15 @@ static void loadDerivations(EvalState & state, const SourcePath & nixExprPath,
}
static long getPriority(EvalState & state, PackageInfo & drv)
static NixInt getPriority(EvalState & state, PackageInfo & drv)
{
return drv.queryMetaInt("priority", 0);
return drv.queryMetaInt("priority", NixInt(0));
}
static long comparePriorities(EvalState & state, PackageInfo & drv1, PackageInfo & drv2)
static std::strong_ordering comparePriorities(EvalState & state, PackageInfo & drv1, PackageInfo & drv2)
{
return getPriority(state, drv2) - getPriority(state, drv1);
return getPriority(state, drv2) <=> getPriority(state, drv1);
}
@ -280,7 +280,7 @@ std::vector<Match> pickNewestOnly(EvalState & state, std::vector<Match> matches)
auto & oneDrv = match.packageInfo;
const auto drvName = DrvName { oneDrv.queryName() };
long comparison = 1;
std::strong_ordering comparison = std::strong_ordering::greater;
const auto itOther = newest.find(drvName.name);
@ -288,9 +288,9 @@ std::vector<Match> pickNewestOnly(EvalState & state, std::vector<Match> matches)
auto & newestDrv = itOther->second.packageInfo;
comparison =
oneDrv.querySystem() == newestDrv.querySystem() ? 0 :
oneDrv.querySystem() == settings.thisSystem ? 1 :
newestDrv.querySystem() == settings.thisSystem ? -1 : 0;
oneDrv.querySystem() == newestDrv.querySystem() ? std::strong_ordering::equal :
oneDrv.querySystem() == settings.thisSystem ? std::strong_ordering::greater :
newestDrv.querySystem() == settings.thisSystem ? std::strong_ordering::less : std::strong_ordering::equal;
if (comparison == 0)
comparison = comparePriorities(state, oneDrv, newestDrv);
if (comparison == 0)
@ -625,13 +625,13 @@ static void upgradeDerivations(Globals & globals,
continue;
DrvName newName(j->queryName());
if (newName.name == drvName.name) {
int d = compareVersions(drvName.version, newName.version);
std::strong_ordering d = compareVersions(drvName.version, newName.version);
if ((upgradeType == utLt && d < 0) ||
(upgradeType == utLeq && d <= 0) ||
(upgradeType == utEq && d == 0) ||
upgradeType == utAlways)
{
long d2 = -1;
std::strong_ordering d2 = std::strong_ordering::less;
if (bestElem != availElems.end()) {
d2 = comparePriorities(*globals.state, *bestElem, *j);
if (d2 == 0) d2 = compareVersions(bestVersion, newName.version);
@ -902,7 +902,7 @@ static VersionDiff compareVersionAgainstSet(
for (auto & i : elems) {
DrvName name2(i.queryName());
if (name.name == name2.name) {
int d = compareVersions(name.version, name2.version);
std::strong_ordering d = compareVersions(name.version, name2.version);
if (d < 0) {
diff = cvGreater;
version = name2.version;