feat: fix some warnings, update PUSH calls

[khushal-banks]

Here is the list of changes i tried for #49

This should not be merged though because i don't think this is the final solution. Creating a PR just for sharing list of current changes.

[jgabaut]

This looks very promising. Altough, the second commit is not going to build (as you said in the commit message).

Would you mind making a separate PR for your suggested minor fixes only (or moving your second commit out of this one)?

Especially the format specifier, since I've looked it up and it seems %td is indeed the correct one for ptrdiff_t. It should also work for when _WIN32 is defined, I guess!

I don't know what to name this PR, for now.

[khushal-banks]

@jgabaut Sir, let's not worry about the length of this #71 PR. Let's discuss this inside and out in steps.. are you free right now?

First of all i want to get rid of ptrdiff_t and switch this library to use size_t instead Then we will use %zu as format specifier or size_t

[jgabaut]

First of all i want to get rid of ptrdiff_t and switch this library to use size_t instead

That won't happen in any way or shape. I'm using signed sizes so that negative values can be used as a trap. Using size_t is not the design of this code.

There's valid fixes in here, but the second commit is simply unrelated to the other changes and could/should be moved into a separate PR.

[khushal-banks]

Would you mind making a separate PR for your suggested minor fixes only

I can do that. But let's just discard this PR after we are done with discussion cum planning. Then, i will submit a new PR with these minor changes plus more minor changes - related to conversion from size_t to ptrdiff_t

Warnings related to conversion can be seen using -Wconversion

[khushal-banks]

I'm using signed sizes so that negative values can be used as a trap.

Understood. Can you tell me where are we using negative size? (optional: just for my better understanding) So, ptrdiff_t will remain as is. okay.

[khushal-banks]

Still, two ideas came out of this:

Consider dropping the implicit cast in _PUSH macros
Try providing a macro not using _Alignof() in order to build with C99
    Perhaps the best way might be always passing [the largest possible alignment requirement for current platform](https://stackoverflow.com/questions/38271072/how-to-determine-maximum-required-alignment-in-c99)

Okay, we were discussing this. If we use largest possible alignment then code will work because there will be extra space available for every allocation for arena right? Why don't you think that would be a huge problem? Can not we just stick with C11 and to use _Alignof

[jgabaut]

I can do that. But let's just discard this PR after we are done with discussion cum planning.

If you want a plan, I'd advise (as I stated before) to make a simpler PR with a clear scope. I didn't really understand what you meant by changes related to conversion.

Then, i will submit a new PR with these minor changes plus more minor changes - related to conversion from size_t to ptrdiff_t Warnings related to conversion can be seen using -Wconversion

Thanks for this one, it seems -Wall doesn't get this one (neither does -Wextra, from what I've seen, but the current Makefile.am does not have -Wextra). Would you mind opening a new bug issue related to this kind of warnings?

[jgabaut]

Understood. Can you tell me where are we using negative size?

Check the code for any kls_push() function, you will see that a negative size should be caught and result in a failure... At least, that's what I think should happen.

[khushal-banks]

Makefile.am does not have -Wextra). Would you mind opening a new bug issue related to this kind of warnings?

I used following.

[khushal-banks]

any kls_push() function, you will see that a negative size should be caught and result in a failure

It seems all of the variables can be positive. Some changes would be required for padding calculation. Notice how padding is intentionally calculated as a negative value

ptrdiff_t padding = -kls->offset & (align - 1);                                                                                               
if (count > PTRDIFF_MAX / size || available - padding < size * count) { 

can be changed to something like (rough estimate .. code is complex)

ptrdiff_t padding = kls->offset & (1 - align);                                                                                               
if (count > PTRDIFF_MAX / size || available + padding < size * count) { 

[khushal-banks]

I didn't really understand what you meant by changes related to conversion.

Compiler is showing following warnings. I want to fix them with these minor changes by making use of size_t. size is always positive for any data type. So, it should be a right step.

[jgabaut]

I am confused. The thing that matters is:

Does the if condition correctly handle count and size even when they're wrong? The padding being used as negative is not necessarily a bad thing.

Remember we don't care about ptrdiff_t negative values, we just use them to catch errors.

As for the multiplication, if it's going to fit into size_t (by using PTRDIFF_MAX in the calcs) and it's never going to turn out that either size or count are negative at that point in the code, is it still a problem?

[jgabaut]

@khushal-banks

I noticed in the failed workflow that the build error for _Alignof(expression) comes from -Wpedantic flag.

Does this mean there's some extensions (not-ISO C) out there allowing this idiom?

[khushal-banks]

Does this mean there's some extensions (not-ISO C) out there allowing this idiom?

If it is not allowed by ISO C then no extensions can allow this. I checked with -std=gnu11 (just to confirm)

[jgabaut]

If it is not allowed by ISO C then no extensions can allow this. I checked with -std=gnu11 (just to confirm)

So trying to build without -Wpedantic leads to undefined behaviour, or am I missing something?

[jgabaut]

Does the if condition correctly handle count and size even when they're wrong? The padding being used as negative is not necessarily a bad thing.

Remember we don't care about ptrdiff_t negative values, we just use them to catch errors.

As for the multiplication, if it's going to fit into size_t (by using PTRDIFF_MAX in the calcs) and it's never going to turn out that either size or count are negative at that point in the code, is it still a problem?

@khushal-banks Please advise, I'm asking for confirmation that the current code is doing meaningful logic. Can we conjure an example where bad input breaks the upper macros or the functions below?

[khushal-banks]

I am confused. The thing that matters is:

Does the if condition correctly handle count and size even when they're wrong? The padding being used as negative is not necessarily a bad thing.

Remember we don't care about ptrdiff_t negative values, we just use them to catch errors.

As for the multiplication, if it's going to fit into size_t (by using PTRDIFF_MAX in the calcs) and it's never going to turn out that either size or count are negative at that point in the code, is it still a problem?

On 32-bit system

size_t and ptrdiff_t both will take 32 bits

• __Range of size_t: 00000000000 to 4294967296
• Range of ptrdiff_t: -2147483648 to 2147483647

On 64-bit system

size_t and ptrdiff_t both will take 64 bits

• ___Range of size_t: 000000000000000000 to 18446744073709551616
• Range of ptrdiff_t: -9223372036854775808 to 9223372036854775807

Read-More So, warning messages are because size_t which is the original return type from both sizeof and _Alignof

So, sizeof and _Alignof may return larges values above PTRDIFF_MAX which is current consideration. Real limit is SIZE_MAX

Problem

Max size possible for a structure is SIZE_MAX Say, we have a structure above size PTRDIFF_MAX of type Example

// macros are passing argument to kls_push... functions like below,
ptrdiff_t size = sizeof(Example)

So, based on Most significant bit value a positive number may be converted to negative. Compiler is warning about this issue.

Question

If you want to claim that first we are limiting the size to a lower value PTRDIFF_MAX then we are catching negative size values in our code logic. Then, all i am saying there should be no negative value.. all sizes are positive.

So, why to limit size to PTRDIFF_MAX by using ptrdiff_t. Use size_t .. always positive. Update calculation logic. Compiler will also be happy. Is this not possible ? What am i missing?

[khushal-banks]

Can we conjure an example where bad input breaks the upper macros or the functions below?

I am confused myself. I am only concerned because i don't like warnings.

If such an example where we create above mentioned comment; Example struct. If kls_push... functions ignores that because of negative size then these warnings can be allowed right?

In previous comment, i am trying to remove warnings from the root

[khushal-banks]

If it is not allowed by ISO C then no extensions can allow this. I checked with -std=gnu11 (just to confirm)

So trying to build without -Wpedantic leads to undefined behaviour, or am I missing something?

I like pedantic it always catches good stuff. Check this doc, it's for strict coding.

[jgabaut]

I understand and support your findings, what I'm asking is what happens if the passed size_t is larger than PTRDIFF_MAX.

• If it's defined behaviour for it to wrap-around and seem negative, it should (please confirm) be catched by the inner logic as an error.
• If it's not going to be like that, what happens? Undefined behaviour?

[jgabaut]

So, sizeof and _Alignof may return larges values above PTRDIFF_MAX which is current consideration. Real limit is SIZE_MAX

Problem

Max size possible for a structure is SIZE_MAX Say, we have a structure above size PTRDIFF_MAX of type Example

// macros are passing argument to kls_push... functions like below,
ptrdiff_t size = sizeof(Example)

So, based on Most significant bit value a positive number may be converted to negative. Compiler is warning about this issue.

Question

If you want to claim that first we are limiting the size to a lower value PTRDIFF_MAX then we are catching negative size values in our code logic. Then, all i am saying there should be no negative value.. all sizes are positive.

So, why to limit size to PTRDIFF_MAX by using ptrdiff_t. Use size_t .. always positive. Update calculation logic. Compiler will also be happy. Is this not possible ? What am i missing?

I just prefer to use ptrdiff_t since it's the recommended type for pointer subtraction, and I prefer the signed size for the trap. I guess someone needing structures bigger than PTRDIFF_MAX is not going to split them up into separate allocations... And it's going to take a different library to solve this issue. A basic arena API can be done in an handful of lines.

Moreover, there's a limit on biggest possible Koliseo size too. Check the big_size test.

All in all, I agree that if such a limit exists/is expected, it should reflect in documentation for this repo. I'll open a separate issue for this later, as soon as I can.

[khushal-banks]

I just prefer to use ptrdiff_t since it's the recommended type for pointer subtraction, and I prefer the signed size for the trap. Moreover, there's a limit on biggest possible Koliseo size too. Check the big_size test.

Okay. This answers all my questions.

I am closing this because this PR was just to refer that #49 failed when used item_ptr instead of type.