Skip to content

Commit 812557b

Browse files
jayschwaandrewrk
authored andcommitted
std: Restore conventional compareFn behavior for binarySearch
PR #20927 made some improvements to the `binarySearch` API, but one change I found surprising was the relationship between the left-hand and right-hand parameters of `compareFn` was inverted. This is different from how comparison functions typically behave, both in other parts of Zig (e.g. `std.math.order`) and in other languages (e.g. C's `bsearch`). Unless a strong reason can be identified and documented for doing otherwise, I think it'll be better to stick with convention. While writing this patch and changing things back to the way they were, the predicates of `lowerBound` and `upperBound` seemed to be the only areas that benefited from the inversion. I don't think that benefit is worth the cost, personally. Calling `Order.invert()` in the predicates accomplishes the same goal.
1 parent 7caa3d9 commit 812557b

File tree

5 files changed

+30
-32
lines changed

5 files changed

+30
-32
lines changed

lib/compiler/aro/aro/Preprocessor.zig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ fn clearBuffers(pp: *Preprocessor) void {
271271
pub fn expansionSlice(pp: *Preprocessor, tok: Tree.TokenIndex) []Source.Location {
272272
const S = struct {
273273
fn orderTokenIndex(context: Tree.TokenIndex, item: Tree.TokenIndex) std.math.Order {
274-
return std.math.order(item, context);
274+
return std.math.order(context, item);
275275
}
276276
};
277277

lib/std/debug/Coverage.zig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ pub fn resolveAddressesDwarf(
196196
const table_addrs = slc.line_table.keys();
197197
line_table_i = std.sort.upperBound(u64, table_addrs, pc, struct {
198198
fn order(context: u64, item: u64) std.math.Order {
199-
return std.math.order(item, context);
199+
return std.math.order(context, item);
200200
}
201201
}.order);
202202
}

lib/std/debug/Dwarf.zig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ pub const CompileUnit = struct {
182182
pub fn findSource(slc: *const SrcLocCache, address: u64) !LineEntry {
183183
const index = std.sort.upperBound(u64, slc.line_table.keys(), address, struct {
184184
fn order(context: u64, item: u64) std.math.Order {
185-
return std.math.order(item, context);
185+
return std.math.order(context, item);
186186
}
187187
}.order);
188188
if (index == 0) return missing();

lib/std/debug/SelfInfo.zig

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1624,12 +1624,12 @@ pub fn unwindFrameDwarf(
16241624
} else {
16251625
const index = std.sort.binarySearch(Dwarf.FrameDescriptionEntry, di.fde_list.items, context.pc, struct {
16261626
pub fn compareFn(pc: usize, item: Dwarf.FrameDescriptionEntry) std.math.Order {
1627-
if (pc < item.pc_begin) return .gt;
1627+
if (pc < item.pc_begin) return .lt;
16281628

16291629
const range_end = item.pc_begin + item.pc_range;
16301630
if (pc < range_end) return .eq;
16311631

1632-
return .lt;
1632+
return .gt;
16331633
}
16341634
}.compareFn);
16351635

lib/std/sort.zig

Lines changed: 25 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,8 @@ pub fn binarySearch(
461461
const mid = low + (high - low) / 2;
462462
switch (compareFn(context, items[mid])) {
463463
.eq => return mid,
464-
.lt => low = mid + 1, // item too small
465-
.gt => high = mid, // item too big
464+
.gt => low = mid + 1,
465+
.lt => high = mid,
466466
}
467467
}
468468
return null;
@@ -471,13 +471,13 @@ pub fn binarySearch(
471471
test binarySearch {
472472
const S = struct {
473473
fn orderU32(context: u32, item: u32) std.math.Order {
474-
return std.math.order(item, context);
474+
return std.math.order(context, item);
475475
}
476476
fn orderI32(context: i32, item: i32) std.math.Order {
477-
return std.math.order(item, context);
477+
return std.math.order(context, item);
478478
}
479479
fn orderLength(context: usize, item: []const u8) std.math.Order {
480-
return std.math.order(item.len, context);
480+
return std.math.order(context, item.len);
481481
}
482482
};
483483
const R = struct {
@@ -489,9 +489,9 @@ test binarySearch {
489489
}
490490

491491
fn order(context: i32, item: @This()) std.math.Order {
492-
if (item.e < context) {
492+
if (context < item.b) {
493493
return .lt;
494-
} else if (item.b > context) {
494+
} else if (context > item.e) {
495495
return .gt;
496496
} else {
497497
return .eq;
@@ -513,9 +513,8 @@ test binarySearch {
513513
try std.testing.expectEqual(2, binarySearch([]const u8, &[_][]const u8{ "", "abc", "1234", "vwxyz" }, @as(usize, 4), S.orderLength));
514514
}
515515

516-
/// Returns the index of the first element in `items` returning `.eq` or `.gt`
517-
/// when given to `compareFn`.
518-
/// - Returns `items.len` if all elements return `.lt`.
516+
/// Returns the index of the first element in `items` that is greater than or equal to `context`,
517+
/// as determined by `compareFn`. If no such element exists, returns `items.len`.
519518
///
520519
/// `items` must be sorted in ascending order with respect to `compareFn`:
521520
/// ```
@@ -540,7 +539,7 @@ pub fn lowerBound(
540539
) usize {
541540
const S = struct {
542541
fn predicate(ctx: @TypeOf(context), item: T) bool {
543-
return compareFn(ctx, item) == .lt;
542+
return compareFn(ctx, item).invert() == .lt;
544543
}
545544
};
546545
return partitionPoint(T, items, context, S.predicate);
@@ -549,13 +548,13 @@ pub fn lowerBound(
549548
test lowerBound {
550549
const S = struct {
551550
fn compareU32(context: u32, item: u32) std.math.Order {
552-
return std.math.order(item, context);
551+
return std.math.order(context, item);
553552
}
554553
fn compareI32(context: i32, item: i32) std.math.Order {
555-
return std.math.order(item, context);
554+
return std.math.order(context, item);
556555
}
557556
fn compareF32(context: f32, item: f32) std.math.Order {
558-
return std.math.order(item, context);
557+
return std.math.order(context, item);
559558
}
560559
};
561560
const R = struct {
@@ -566,7 +565,7 @@ test lowerBound {
566565
}
567566

568567
fn compareFn(context: i32, item: @This()) std.math.Order {
569-
return std.math.order(item.val, context);
568+
return std.math.order(context, item.val);
570569
}
571570
};
572571

@@ -584,9 +583,8 @@ test lowerBound {
584583
try std.testing.expectEqual(2, lowerBound(R, &[_]R{ R.r(-100), R.r(-40), R.r(-10), R.r(30) }, @as(i32, -20), R.compareFn));
585584
}
586585

587-
/// Returns the index of the first element in `items` returning `.gt`
588-
/// when given to `compareFn`.
589-
/// - Returns `items.len` if none of the elements return `.gt`.
586+
/// Returns the index of the first element in `items` that is greater than `context`, as determined
587+
/// by `compareFn`. If no such element exists, returns `items.len`.
590588
///
591589
/// `items` must be sorted in ascending order with respect to `compareFn`:
592590
/// ```
@@ -611,7 +609,7 @@ pub fn upperBound(
611609
) usize {
612610
const S = struct {
613611
fn predicate(ctx: @TypeOf(context), item: T) bool {
614-
return compareFn(ctx, item) != .gt;
612+
return compareFn(ctx, item).invert() != .gt;
615613
}
616614
};
617615
return partitionPoint(T, items, context, S.predicate);
@@ -620,13 +618,13 @@ pub fn upperBound(
620618
test upperBound {
621619
const S = struct {
622620
fn compareU32(context: u32, item: u32) std.math.Order {
623-
return std.math.order(item, context);
621+
return std.math.order(context, item);
624622
}
625623
fn compareI32(context: i32, item: i32) std.math.Order {
626-
return std.math.order(item, context);
624+
return std.math.order(context, item);
627625
}
628626
fn compareF32(context: f32, item: f32) std.math.Order {
629-
return std.math.order(item, context);
627+
return std.math.order(context, item);
630628
}
631629
};
632630
const R = struct {
@@ -637,7 +635,7 @@ test upperBound {
637635
}
638636

639637
fn compareFn(context: i32, item: @This()) std.math.Order {
640-
return std.math.order(item.val, context);
638+
return std.math.order(context, item.val);
641639
}
642640
};
643641

@@ -780,16 +778,16 @@ pub fn equalRange(
780778
test equalRange {
781779
const S = struct {
782780
fn orderU32(context: u32, item: u32) std.math.Order {
783-
return std.math.order(item, context);
781+
return std.math.order(context, item);
784782
}
785783
fn orderI32(context: i32, item: i32) std.math.Order {
786-
return std.math.order(item, context);
784+
return std.math.order(context, item);
787785
}
788786
fn orderF32(context: f32, item: f32) std.math.Order {
789-
return std.math.order(item, context);
787+
return std.math.order(context, item);
790788
}
791789
fn orderLength(context: usize, item: []const u8) std.math.Order {
792-
return std.math.order(item.len, context);
790+
return std.math.order(context, item.len);
793791
}
794792
};
795793

0 commit comments

Comments
 (0)