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

Add the CTypes module for common C type aliases #14448

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

# fun bar(x : CTypes::Char*) # okay
# end
# ```
module CTypes
Copy link
Member

@straight-shoota straight-shoota Apr 6, 2024

Choose a reason for hiding this comment

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

thought: I was wondering if module C could be an option as it allows to conveniently write C::Char. But that would likely cause confusion because A, B, C are commonly used as stub constants.

Importing with a local alias (private alias C = ::CTypes) would be of similar convenience.

(ref #13504 (comment) ff.)


# The C `intptr_t` type.
#
# Large enough to hold the value of `Pointer#address`. Equivalent to `Int32`
Copy link
Member

Choose a reason for hiding this comment

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

question: Is this actually true? Isn't Pointer#address currently 64-bits on all architectures?

Copy link
Contributor Author

@HertzDevil HertzDevil Apr 6, 2024

Choose a reason for hiding this comment

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

It is more like "CTypes::UIntPtrT.new(ptr.address) should never raise OverflowError" on any target. And this should be true even if ptr = Pointer(Void).new(UInt64::MAX) on a 32-bit platform, as LibLLVM.build_int2ptr will zero-extend or truncate the address.

@ysbaddaden
Copy link
Contributor

As stated in the issue: I fail to see the point. What's wrong with LibC::SizeT?

@Blacksmoke16
Copy link
Member

One of the big benefits of this IMO is having it actually show up in the API docs. Tho a related problem of that is the API docs are essentially going to be "wrong" in that they'll only be applicable to whatever built them.

An alternative would be having something in the reference material for this.

@Sija
Copy link
Contributor

Sija commented Apr 15, 2024

If the docs are being arch specific it might lead to confusion, reference material would be IMO clearer in this regard.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 15, 2024

@Blacksmoke16 Good point. With normal constants we can actually have the generic definition including the macro code that decides the value based on different flags (example File::NULL).
But this doesn't work with alias (Maybe it could, though? I think the API docs for aliases could be improved 🤔).

Anyway, the doc comments in this PR are pointing out the options, so I don't think it's a big issue.

@HertzDevil
Copy link
Contributor Author

With alias Int = {% if ... the whole condition shows up in the docs but Int cannot be used in top-level macros (it would evaluate to that MacroIf node). With {% if ... %} alias Int = ... it is the opposite.

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

Successfully merging this pull request may close these issues.

5 participants