Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardized geometries and unsigned dimensions #2331

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

dankamongmen
Copy link
Owner

@dankamongmen dankamongmen commented Nov 7, 2021

As discussed in #1696, #2324, and #2320, the default semantics for arguments are now:

  • target coordinate/upper-left origin: signed integers. -1 means "cursor position in this dimension". less than -1 is an error. non-negative numbers must be within the plane.
  • area: unsigned integers. 0 means "through the remaining right/below material".
  • lower-right corner: unused. whenever an area is necessary, it ought be origin + vector

Furthermore, most dimensions are now unsigned, whether return values or parameters, fulfilling one of @joseluis's dreams as old as time. There might be further conversions of this nature.

Remove recursion from ncplane_polyfill_yx() just as we did ncvisual_polyfill_yx() a few days ago, to defend against small stacks.

Use ncmetric() in the HUD of notcurses-demo.

This is a tremendous amount of churn. I think it improves things overall, but goddamn, there are something like a hundred files touched here. The NOTES update is almost certainly incomplete. I probably missed a function or two for conversion. I would be shocked if I have not introduced at least three subtle bugs with this PR. Please give it a scan, watching for things like copy-and-paste bugs etc.

@igo95862 , I still need to convert the python. If you could get to it, that would be awesome; if not, I'll handle it. There are still a great many compiler warnings being issued which I must remedy before merging.

@igo95862
Copy link
Contributor

igo95862 commented Nov 7, 2021

I still need to convert the python. If you could get to it, that would be awesome; if not, I'll handle it.

Sorry I have not been following the project. (busy with work and my own projects)

https://docs.python.org/3/c-api/arg.html#numbers

All the PyArg_ParseTuple format lines should also be updated to indicate the conversion to unsigned int.

PS. I did not expect this project API to change so rapidly when I first joined. (usually breaking changes are reserved for x.0.0 major versions)

@dankamongmen
Copy link
Owner Author

PS. I did not expect this project API to change so rapidly when I first joined. (usually breaking changes are reserved for x.0.0 major versions)

.nodnod, we're doing the 3.0.0 release in two weeks, and all of this is prep for that move to abi3.

@dankamongmen
Copy link
Owner Author

alright, i just converted all the python, and i eliminated all compiler warnings earlier today. let's see if we get green builds...

@dankamongmen dankamongmen force-pushed the dankamongmen/negativecursor branch from b6eb461 to e10a8fb Compare November 9, 2021 00:59
@dankamongmen
Copy link
Owner Author

136 files changed, jesus

@dankamongmen dankamongmen force-pushed the dankamongmen/negativecursor branch from 5e2c632 to cac98ed Compare November 9, 2021 01:12
@dankamongmen
Copy link
Owner Author

alright, everything looks about as good as it's going to look, i suspect. are we doing this?

@dankamongmen dankamongmen force-pushed the dankamongmen/negativecursor branch from cac98ed to 2c67da9 Compare November 9, 2021 01:39
@dankamongmen
Copy link
Owner Author

so the only real question is whether we want to cut a 2.4.9 before merging this. once it's merged, i think a 2.4.9 is largely impossible, and we ought plow ahead to 3.0.0 ASAP. the only downside of cutting a 2.4.9 now would be the earlier unsigned work. maybe i ought make a branch from right before that, and cherry-pick a few things? hrmmm.

@dankamongmen
Copy link
Owner Author

so the only real question is whether we want to cut a 2.4.9 before merging this. once it's merged, i think a 2.4.9 is largely impossible, and we ought plow ahead to 3.0.0 ASAP. the only downside of cutting a 2.4.9 now would be the earlier unsigned work. maybe i ought make a branch from right before that, and cherry-pick a few things? hrmmm.

i made a branch 2.x.x from 3bdbf6e, and cherry-picked a number of recent changes. i might cut this as 2.4.9.

@dankamongmen
Copy link
Owner Author

fire in the hole!

@dankamongmen dankamongmen merged commit 12e01fd into master Nov 9, 2021
@dankamongmen dankamongmen deleted the dankamongmen/negativecursor branch November 9, 2021 02:05
@joseluis
Copy link
Collaborator

joseluis commented Nov 10, 2021

any chance to change the type of sbytes in the ncplane_putegc_yx & ncplane_putegc_stained functions from an int to an uint or even better a uintptr_t?

@dankamongmen
Copy link
Owner Author

any chance to change the type of sbytes in the ncplane_putegc_yx & ncplane_putegc_stained functions from an int to an uint or even better a uintptr_t?

probably. you don't want uintptr_t; that's "an unsigned which is large enough to hold a pointer". you just want unsigned *.

@dankamongmen
Copy link
Owner Author

also it would probably be size_t *.

@dankamongmen
Copy link
Owner Author

any chance to change the type of sbytes in the ncplane_putegc_yx & ncplane_putegc_stained functions from an int to an uint or even better a uintptr_t?

done in master. this will not be backported to 2.x.x.

@joseluis
Copy link
Collaborator

any chance to change the type of sbytes in the ncplane_putegc_yx & ncplane_putegc_stained functions from an int to an uint or even better a uintptr_t?

probably. you don't want uintptr_t; that's "an unsigned which is large enough to hold a pointer". you just want unsigned *.

Mmmm I really meant "an unsigned large enough to hold a pointer" thinking on the equivalent to rust's usize which is used for indexing into arrays...

But this particular type translation is somewhat confusing and I'm see a couple of problems.

Currently the type definition of size_t coming from bindgen is a cty::c_ulong which is a u64.

pub type size_t = cty::c_ulong;

in cty:

pub type c_ulong = u64;

but in cty, a size_t is a usize!

pub type size_t = usize;

Also look at this rust-lang/rust-bindgen#1903

Maybe updating our version of bindgen (from 0.57) would help this (not sure) but I think we couldn't update yet because higher versions where not available on certain platforms/distributions?

For now I'm gonna go with how things are right now, and try to make it work without any more changes.

@joseluis
Copy link
Collaborator

More background:

After reading some discussions I've enabled the option size_t_is_usize for bindgen and it seems to easy things out a little in this regard. Let's cross fingers.

@dankamongmen
Copy link
Owner Author

Mmmm I really meant "an unsigned large enough to hold a pointer" thinking on the equivalent to rust's usize which is used for indexing into arrays...

an index into an array is not a pointer; the array itself is the pointer, and the index is...an index. multiplied by the size of the data being pointed to, it is an offset.

arrays and pointers as arguments in C are pretty much the same thing. you never copy an array as an argument, so

char a[5];
foo(a);

is only passing a char*. what we have here is a block of 5 bytes (sizeof(char) == 1, as guaranteed by the language). that block is pointed to by a, which cannot change its value (i.e. cannot point at anything else). note that this is completely legal and regular:

char* b = a;

b, a pointer to char, is now pointing at the char pointed to by a, and can be safely dereferenced as b[0]..b[4].

i would write foo ala:

void foo(char* crap);

or

void foo(const char* crap)

i could also unidiomatically write:

void foo(const char crap[]);

but people would look at me weirdly.

remember -- C supports only one parameter-passing mechanism, call by value. if i want to change the value in my external scope, i must pass a pointer. so if i have

unsigned foo;
bar(foo);

foo is impossible to change in bar (assuming no aliasing etc.) because i'm only ever passing a copy of its value. if i instead do

unsigned foo;
bar(&foo);

together with

void bar(unsigned* qux);

i can now modify *qux aka foo in bar because i'm changing the value being pointed at. but void bar(unsigned qux) cannot affect foo. so we want a value which is large enough to be an index into any array, which is size_t (the input type to malloc). we want to be able to modify that index, so we need size_t*. and there you go.

uintptr_t is something different. that's when you need an integer type into which you can cast any pointer without loss of information. so yes technically you could do either of:

void quuux_good(size_t* arp){
  *arp = 5;
}

void quuux_bad(uintptr_t arp){
  *(size_t *)arp = 5;
}

size_t vroom;
quuux_good(&vroom);
quuux_bad((uintptr_t)&vroom);

but the latter throws away just about all type information, losing you performance and safety, and requiring casts all over the place, which are very much to be avoided.

@dankamongmen
Copy link
Owner Author

uintptr_t is something different. that's when you need an integer type into which you can cast any pointer without loss of information. so yes technically you could do either of:

void quuux_good(size_t* arp){
  *arp = 5;
}

void quuux_bad(uintptr_t arp){
  *(size_t *)arp = 5;
}

size_t vroom;
quuux_good(&vroom);
quuux_bad((uintptr_t)&vroom);

but the latter throws away just about all type information, losing you performance and safety, and requiring casts all over the place, which are very much to be avoided.

you might want a uintptr_t if like you're writing a debugger and need to read a memory address encoded as ASCII from input. you'd parse that up as a uintptr_t and then cast it to whatever pointer type you wished to interpret it as.

@dankamongmen
Copy link
Owner Author

that guy in bindgen #1903 seems a bit confused as to the relationship of size_t and uintptr_t IMHO. size_t absolutely does not need to be equal in size to uintptr_t. on a segmented architecture, for instance, you can't usually have an array larger than the segment size. size_t only needs be large enough to handle any array offset. uintptr_t needs be able to handle any pointer.

@dankamongmen
Copy link
Owner Author

dankamongmen commented Nov 11, 2021

https://en.cppreference.com/w/c/types/size_t

size_t can store the maximum size of a theoretically possible object of any type (including array). size_t is commonly used for array indexing and loop counting. Programs that use other types, such as unsigned int, for array indexing may fail on, e.g. 64-bit systems when the index exceeds UINT_MAX or if it relies on 32-bit modular arithmetic.

https://en.cppreference.com/w/cpp/types/integer

uintptr_t: unsigned integer type capable of holding a pointer

i don't understand why https://internals.rust-lang.org/t/pre-rfc-usize-is-not-size-t/15369 seems to conflate ptrdiff_t and uintptr_t. they're not at all the same thing. ptrdiff_t is the difference between two pointers into the same array. it does not need cover all possible pointers. you are not allowed to arbitrarily compare or subtract two pointers in C; it is only meaningful to compare pointers within the same array plus one.

int a;
int b;
int* pa = &a;
int* pb = &b;
ptrdiff_t d = pa - pb;

this is illegal. pa and pb are not comparable. thus ptrdiff_t never needs to be larger than size_t. uintptr_t may need to be larger than size_t. that RFC has some very loose, very questionable language.

@dankamongmen
Copy link
Owner Author

https://en.cppreference.com/w/cpp/types/ptrdiff_t

ptrdiff_t is used for pointer arithmetic and array indexing...Only pointers to elements of the same array (including the pointer one past the end of the array) may be subtracted from each other. If an array is so large (greater than PTRDIFF_MAX elements, but less than SIZE_MAX bytes), that the difference between two pointers may not be representable as ptrdiff_t, the result of subtracting two such pointers is undefined. For char arrays shorter than PTRDIFF_MAX, ptrdiff_t acts as the signed counterpart of size_t it can store the size of the array of any type and is on most platforms synonymous with intptr_t.

@dankamongmen
Copy link
Owner Author

basically just about every correct C program in the world makes use of size_t; i have made use of uintptr_t maybe five times in ~30 years of writing C.

@joseluis
Copy link
Collaborator

an index into an array is not a pointer; the array itself is the pointer, and the index is...an index. multiplied by the size of the data being pointed to, it is an offset.

size_t only needs be large enough to handle any array offset. uintptr_t needs be able to handle any pointer.

uintptr_t is something different. that's when you need an integer type into which you can cast any pointer without loss of information

Ok. I see. I'm grateful Rust just uses an usize for that.

i could also unidiomatically write: void foo(const char crap[]); but people would look at me weirdly.

funny how culture goes. I like that one better because it gives more information of intent.

this is illegal. pa and pb are not comparable. thus ptrdiff_t never needs to be larger than size_t. uintptr_t may need to be larger than size_t. that RFC has some very loose, very questionable language.

maybe you want to leave them a msg with your perspective?

Thanks for all the awesome insider info about C nick!

@dankamongmen
Copy link
Owner Author

funny how culture goes. I like that one better because it gives more information of intent.

how so? i cannot think of any way in which it does.

btw since c99 you can use crap[static 5] to indicate to static analyzers that you want at least 5 available; as far as i'm aware, it in no way actually ties into the typing system (arrays are not a type in C).

@joseluis
Copy link
Collaborator

how so? i cannot think of any way in which it does.

by using one syntax to indicate an array is expected, and the other one to indicate a single value is expected? that's what I was thinking anyway

@dankamongmen
Copy link
Owner Author

by using one syntax to indicate an array is expected, and the other one to indicate a single value is expected? that's what I was thinking anyway

ctype

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eliminate recursion in ncplane_polyfill_yx() coordinate arguments ought always accept -1 as "current cursor"
3 participants