refactor cvector_at

[SekaiArendelle]

After I closed my issue, I suddenly had an idea.

Now, cvector_at can be used as follow:

#include "cvector.h"
#include <stdio.h>
#include <assert.h>

int main(void) {
    cvector(int) v = NULL;
    cvector_push_back(v, 1);

    printf("%d\n", cvector_at(v, 0)); // get value
    cvector_at(v, 0) = 2; // set value
    printf("%d\n", cvector_at(v, 0));
    // printf("%d\n", cvector_at(v, 1)); // assert fail

    return 0;
}

[eteran]

Hmm, interesting usage of assert and comma operator there. I'll take a closer look in a little while

[SekaiArendelle]

I have fixed all the test(including some bug of test e.g. calloc(sizeof(struct data_t), 1)

[eteran]

Can you explain what you believe was broken about:

struct data_t *data = calloc(sizeof(struct data_t), 1);

vs what you changed it to:

struct data_t *data = calloc(1, sizeof(struct data_t));

Given that the nmem and size parameters are basically just multiplied (and checked for overflow), this should make no difference. I don't really mind the change, I was just curious about the reasoning.

[eteran]

Also, it appears that your PR requires adding an #include to the cvector.h file.

[SekaiArendelle]

Can you explain what you believe was broken about:

The compiler gives me warning.

void *__cdecl calloc(size_t _NumOfElements,size_t _SizeOfElements);

[eteran]

Thanks!

[smlavine]

This has been merged, but I propose that this change be reverted. Previously, cvector_at could also be used as a concise way to see if there was an element at the given index (i.e. checking the size), in addition to accessing it. Like so:

int *f = cvector_at(v, 4);
if (f) {
    *f = 1234; // ...
}

With the current API, we have to do this, which gives the library user fewer options and IMO is more awkward:

if (cvector_size(v) > 4) {
    v[4] = 1234; // ....
}

With this cvector_at, there is now no way to get a reference to an arbitrary location in the vector, meaning it is more awkward to change values of elements within the vector.

Before this change, if you wanted to get the value of some index in the vector you could just use the v[n] syntax. Why does this need to be duplicated in cvector_at as well?

Also, I believe this broke/changed cvector_front/cvector_back. Those are documented as giving references to the first/last element in the vector, but since they relied on cvector_at, they now return the value, not the reference. Either this implementation must be changed, or the documentation must be updated.

Thanks for the great library! I'd be happy to submit a PR reverting this.

[SekaiArendelle]

Hmm, I don't think there are any difference between

int *f = cvector_at(v, 4);
if (f) {
*f = 1234; // ...
}

and

if (cvector_size(v) > 4) {
v[4] = 1234; // ....
}

In my opinion, writing out of bounds of an array is an unrecoverable error that should cause the program to crash immediately, alerting the programmer to a fatal issue instead of throwing an exception (especially it's error code).

But if you do want to revert it, I don't mind. The only reason I'm using this library is to complete my homework. I rarely write pure C because it lacks abstraction capabilities. For example, using macros to embed code in C (like with cvector) can lead to binary bloat.

[eteran]

Hmm, interesting thoughts on both sides. First and foremost, if front and back are currently broken, we need to fix that!

Beyond that, I'll have to think on which version of the API I think is better because you both make a compelling argument.

So please, if it's not too much effort, make a PR for the revert so if I decide to revert, it's ready to go.

[SekaiArendelle]

my opinions are as follow:

1. Aborting the program is preferable to returning an error code. It helps avoid the propagation of errors and makes debugging easier by highlighting the exact point of failure.
2. return value instead of it's pointer is closer than the usage of vector::at
3. Checking index < cvector_size(arr) is clearer and more intuitive than using cvector_at and then checking whether the result is NULL. The former approach directly addresses the bounds-checking issue in a more readable manner.
4. do checks if you do not want program crashing instead of "catch" it, which is not a good usage of exception.
5. the current version of cvector_at can also easily get the pointer of elements.

[smlavine]

So please, if it's not too much effort, make a PR for the revert so if I decide to revert, it's ready to go.

Sure, I'll see if I have time to get that out in the next few days.

[smlavine]

Some counterarguments:

1. Aborting the program is preferable to returning an error code. It helps avoid the propagation of errors and makes debugging easier by highlighting the exact point of failure.

I disagree with this in principle, I think that handling "errors" gracefully is more important in any program of significant size, especially in C where the abort procedures are fairly limited. With cvector_at returning a reference (and NULL on out-of-bounds index), it's not so much that we're checking for an error per se, as it wouldn't be "wrong" to call cvector_at with an out-of-bounds index. It would just need to be checked.

2. return value instead of it's pointer is closer than the usage of vector::at

C and C++ are different languages. I personally haven't used C++ in years. Just because a C++ library is designed a certain way should have little bearing on how to design a C library. Exceptions, templates, and RAII are all factors in the decision of how to design a C++ library. Those decisions shouldn't carry over to a C library without good reason.

3. Checking index < cvector_size(arr) is clearer and more intuitive than using `cvector_at` and then checking whether the result is NULL. The former approach directly addresses the bounds-checking issue in a more readable manner.

Maybe, but in the use-case that prompted me to write this, that's not how I was thinking about the problem. An explicit size check asks the question: "Is the vector at least this big?" But calling and checking cvector_at asks: "Does the vector have an element at this index?" With cvector_at returning a reference, I can think about my program in the second way, and then mutate that element. Not so with the value-return implementation.

4. do checks if you do not want program crashing instead of "catch" it, which is not a good usage of exception.

I agree, the if (p) ... is an example of this in C, I think.

5. the current version of cvector_at can also easily get the pointer of elements.

Eh, kinda, but I don't really like the idea of referencing the result of a macro like that. &cvector_at(v, n) works because of the implementation detail of macros, but doesn't vibe with the idea that we are "returning a value". Unless there's another way I'm missing?

[SekaiArendelle]

哎，累了，毁灭吧，赶紧的。（这是一个梗
从程序的控制流上来说，我不认为有什么区别。基于控制流衍生出来的语意，每个人都有不同的理解，这是很正常的（并且每个人心目中的最佳工程实践也是不同的）
以前版本的cvector_at与现在版本的cvector_at，我都不认为有什么质的区别，甚至并不能说哪个版本就一定更好，因为你说的也有道理。
因此，让eteran决定吧

[smlavine]

See #76.

[eteran]

@smlavine I am inclined to agree with your position, thanks for the PR.

@GoodenoughPhysicsLab I appreciate both your efforts to improve the library and your understanding that there is no solution that will seem best to everyone. As I said, I agree with @smlavine views, but that isn't to say that you're "wrong", only that his suggestion feels more natural to me.

Thanks again to both of you, i'll be merging #76 shortly.

[eteran]

Also, @smlavine small world! I went to RIT as well (a long time ago though...)

[SekaiArendelle]

@GoodenoughPhysicsLab I appreciate both your efforts to improve the library and your understanding that there is no solution that will seem best to everyone. As I said, I agree with @smlavine views, but that isn't to say that you're "wrong", only that his suggestion feels more natural to me.

I also appreciate your efforts to this library and your friendly sentence and I actually learned some knowledge from it.

You may feel curious to my extreme terminating-style checking. The most important reason I guessed is that I against using C++ exception but it's the core to cpp because I can't return error code in constructor or overload function, which forced me to terminate the program if some errors have been detected. Another reason is that Herb Sutter have given a report in cppcon2019 and he negated std::logic_error because he thought that exception is a recoverable error, which means when the abstract machine is broken, people can't recover it by catching std::logic_error. I agree with Herb Sutter's thinking and expand it to every unrecoverable error like writing out of bounds of an array. But now, I think that terminating the program just awkward users, which is not good.

Thanks for the great library!

[smlavine]

On Thu Feb 13, 2025 at 1:55 AM EST, Evan Teran wrote: Also, @smlavine small world! I went to RIT as well (a long time ago though...)

No way! Go Tigers. :)