phobos icon indicating copy to clipboard operation
phobos copied to clipboard

Implement opCmp for Nullable objects

Open DeterminedSage opened this issue 10 months ago • 2 comments

Fixes : #10743

Summary

This PR adds a opCmp implementation for Nullable, enabling it to participate in standard algorithms like sort, uniq, and allowing direct comparison with raw values or other nullable-like types.

Root Cause

Previously, 'Nullable' lacked a flexible 'opCmp' method that allowed it to be compared with another 'Nullable' or 'Nullable'-like objects with '.isNull' and '.get' .

This limitation meant that generic code and algorithms relying on opCmp (e.g., sort, uniq, associative containers) could not work with Nullable values out of the box.

For example :

import std.algorithm : sort;

auto arr = [Nullable!int(5), Nullable!int(), Nullable!int(10)];
sort(arr); // compile error before

would fail to compile due to missing comparison support between Nullable instances .

Fix

This patch introduces a generic opCmp(this This, Rhs) function with appropriate constraints to ensure type safety and correct behavior .

int opCmp(this This, Rhs)(auto ref Rhs rhs) const
if (is(typeof(_value.payload < rhs.get)) && is(typeof(_value.payload > rhs.get)))

The implementation handles :

Nullable vs Nullable: compares null-ness, then payload.

Nullable vs nullable-like: uses .isNull and .get.

Nullable vs raw value: directly compares to rhs.

Null is considered less than any non-null value, and two nulls are considered equal.

Example Reproducer

import std.algorithm : sort;
import std.typecons : Nullable;

void main() {
    auto arr = [Nullable!int(10), Nullable!int(), Nullable!int(5)];
    sort(arr);
    assert(arr[0].isNull);
    assert(arr[1].get == 5);
    assert(arr[2].get == 10);
}

This now compiles and runs as expected. Previously, this would result in a compile-time error.

Notes

Unittest addition for basic comparison between null and non-null values

Verified locally using custom Phobos build and test suite.

This patch fully resolves the issue described in #10743.

DeterminedSage avatar Apr 24 '25 18:04 DeterminedSage

Buildkite failure looks unrelated: https://github.com/gecko0307/dagon/issues/106

0xEAB avatar Apr 25 '25 00:04 0xEAB

Buildkite failure looks unrelated: gecko0307/dagon#106

@PetarKirov went for a round 2 of Testing without making any changes but fails again due to same reason

DeterminedSage avatar Apr 25 '25 04:04 DeterminedSage

This PR has had a broken CI for 6+ months, please reopen if you plan to continue working on this.

LightBender avatar Oct 27 '25 02:10 LightBender