Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 10 additions & 26 deletions Vector.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ in the source distribution for its full text.
#include "XUtils.h"


static Object_Compare vectorCompareFn;

Vector* Vector_new(const ObjectClass* type, bool owner, int size) {
Vector* this;

Expand Down Expand Up @@ -95,38 +97,15 @@ void Vector_prune(Vector* this) {

//static int comparisons = 0;

/*
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't keep commented-out code around. That's what a VCS is for …

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would have to delete the commented-out combSort code as well...
Would @hishamhm comment on this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to provide historical commentary, but I defer to the current maintainers on all things code review!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combSort vs. insertionSort place is a different part of the code.

But as sorting has currently two implementations, this is some other cleanup task (#1785) anyway …

static void swap(Object** array, int indexA, int indexB) {
assert(indexA >= 0);
assert(indexB >= 0);
Object* tmp = array[indexA];
array[indexA] = array[indexB];
array[indexB] = tmp;
}

static int partition(Object** array, int left, int right, int pivotIndex, Object_Compare compare) {
const Object* pivotValue = array[pivotIndex];
swap(array, pivotIndex, right);
int storeIndex = left;
for (int i = left; i < right; i++) {
//comparisons++;
if (compare(array[i], pivotValue) <= 0) {
swap(array, i, storeIndex);
storeIndex++;
}
}
swap(array, storeIndex, right);
return storeIndex;
}

static void quickSort(Object** array, int left, int right, Object_Compare compare) {
if (left >= right)
return;

int pivotIndex = (left + right) / 2;
int pivotNewIndex = partition(array, left, right, pivotIndex, compare);
quickSort(array, left, pivotNewIndex - 1, compare);
quickSort(array, pivotNewIndex + 1, right, compare);
}
*/

// If I were to use only one sorting algorithm for both cases, it would probably be this one:
/*
Expand Down Expand Up @@ -167,10 +146,15 @@ static void insertionSort(Object** array, int left, int right, Object_Compare co
}
}

static int Vector_compareObjects(const void* v1, const void* v2) {
return vectorCompareFn(*(const Object* const*)v1, *(const Object* const*)v2);
}

void Vector_quickSortCustomCompare(Vector* this, Object_Compare compare) {
assert(compare);
assert(Vector_isConsistent(this));
quickSort(this->array, 0, this->items - 1, compare);
vectorCompareFn = compare;
qsort(this->array, this->items, sizeof(Object*), Vector_compareObjects);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it weren't for qsort_r only being standardized in POSIX.2024, I'd opt for using it; avoiding the need for global variables. Speaking of which: the wrapper/adapter should check the vectorCompareFn for NULL (marked unlikely to the optimizer).

FWIW: If we wanna be fancy, we could detect a compatible qsort_r implementation in configure.ac and prefer that if available; falling back to qsort only if stars don't align.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish to keep the changes small and leave the qsort_r support to a separate commit.

POSIX standardised the GNU version of qsort_r but before GNU, FreeBSD had a version of qsort_r that was ABI-incompatible. Although checking for qsort_r is possible in configure, it's not trivial code. Given that we still need to support platforms without qsort_r, I consider this qsort_r thing a lower priority.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Exactly because qsort_r support is such a mess I'd suggested to skip it for now and revisit it later..

Which leaves the one NULL check I mentioned above as an item TBD …

Copy link
Copy Markdown
Contributor Author

@Explorer09 Explorer09 Oct 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenBE Yes. I plan to add an assert(!vectorCompareFn) before assignment in order to catch whether Vector_*Sort is called in a signal handler or multithreaded code by mistake.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I was referring to. And also won't actually help except as a canary.

assert(Vector_isConsistent(this));
}

Expand Down
Loading