Add queue module

[briandowns]

Well detailed description of the change :

Added a module providing a queue.

Resolves: #

Type of change:

• Bug fix

• New feature

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Housekeeping

• Tests have been updated to reflect the changes done within this PR (if applicable).

• Documentation has been updated to reflect the changes done within this PR (if applicable).

Preview (Screenshots) :

[briandowns]

Not really sure why CI is failing. Seems to exit without a message. Runs fine locally.

[Jason2605]

Seems like the tests are failing for me locally, will try see what I can find

[Jason2605]

Fixed the segfault but it seems some of the tests are failing / some memory isn't being cleared up, will continue looking

[briandowns]

That's really annoying I'm not seeing that on my local. I'm happy to pull down the segfault fix and keep troubleshooting.

[Jason2605]

Some comments as well about the actual API - I think providing a queue size could probably be optional and we resize it under the hood for the user (similar to how lists work) - what do you think?

I would probably also expect .size() (or maybe we provide a .len()) to give us the amount of items on the queue rather than the underlying size of malloc'd items, this would allow a user to iterate over a queue and pop as we need

[briandowns]

Do we need to account for different architectures in tests? I'm seeing some really weird things in CI that don't quite make sense.

[Jason2605]

Do we need to account for different architectures in tests? I'm seeing some really weird things in CI that don't quite make sense.

Sorry somehow completely missed this comment? The tests for a queue module shouldn't need to be different per architecture no, it would only be when we use an architecture specific include I would have imagined. Are you meaning the failing tests in CI?

[briandowns]

Do we need to account for different architectures in tests? I'm seeing some really weird things in CI that don't quite make sense.

Sorry somehow completely missed this comment? The tests for a queue module shouldn't need to be different per architecture no, it would only be when we use an architecture specific include I would have imagined. Are you meaning the failing tests in CI?

Yeah. Not sure where those errors are coming from. Of course, it works locally. :) Probably something dumb.

[Jason2605]

That should be them passing now @briandowns, was just missing the ->capacity initialiser!

I'm curious too - 99% of the time I would imagine the end user not actually caring about the initial capacity size of the queue, so for this reason I suggest we:

• Have .new() be a default capacity size (probably 8?) then it means we don't need to worry about passing a parameter and it also means we don't need to worry about unwrapping the queue object (saves a little but of verbosity)
• Have a .newWithSize() (or similar name) which is essentially what .new() is now which will return a Result<Queue> type

Thoughts?

[briandowns]

That should be them passing now @briandowns, was just missing the ->capacity initialiser!

I'm curious too - 99% of the time I would imagine the end user not actually caring about the initial capacity size of the queue, so for this reason I suggest we:

• Have .new() be a default capacity size (probably 8?) then it means we don't need to worry about passing a parameter and it also means we don't need to worry about unwrapping the queue object (saves a little but of verbosity)
• Have a .newWithSize() (or similar name) which is essentially what .new() is now which will return a Result type

Thoughts?

Dig it. I'll get on it this evening. I don't know why this was working for me. Thank you!

[Jason2605]

That's all good! Also seem to be getting some strange behaviour in the REPL ;(

Dictu Version: 0.27.0
>>> import Queue;
>>> const x = Queue.new(8).unwrap();
>>> x.push("test");
<Result Suc>
>>> const y = x.pop();
>>> print(y.unwrap());

>>> print(y.unwrap());
dictu(56734,0x11047fdc0) malloc: can't allocate region
:*** mach_vm_map(size=18446744073187381248, flags: 100) failed (error code=3)
dictu(56734,0x11047fdc0) malloc: *** set a breakpoint in malloc_error_break to debug

I'll do some digging into this

Update: Ah I see, if (queue->count < queue->capacity) { in pop will always be true so it's always going to resize and at this point the value doesn't exist in the queue anymore (or on the stack) so it's getting freed

[Jason2605]

Suggested change

	if (queue->count < queue->capacity) {
	int oldCapacity = queue->capacity;
	queue->capacity = SHRINK_CAPACITY(oldCapacity);
	queue->dq = SHRINK_ARRAY(vm, queue->dq, Value, oldCapacity, queue->capacity);
	}
	return newResultSuccess(vm, data);
	return data;

We should definitely think about sizing down queue sizes if it meets a certain threshold, but we wouldn't want it to happen each call to pop() (as count is always going to be less than capacity in this case)

Something to think about with lists too actually, I don't think they size down

[briandowns]

Curious your thoughts on this. I changed the shrink operation to only shrink the underlying array if the count is less than half the capacity but greater than the default size.

[Jason2605]

I like it yep!

[Jason2605]

Sorry last round of comments !

[briandowns]

More HTTP failures. I'd restart but I don't have that option to do so in the interface.

[Jason2605]

Thanks very much for this!