-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move LibC
into Crystal::System
namespace
#13504
Comments
The types like |
Ay, that's a good point. I hadn't intended that. In fact, you need them in any lib type. Thus it's common to have aliases like these for convenience: crystal/src/crystal/lib_iconv.cr Lines 11 to 13 in b2498a3
I'm wondering if there could be alternatives to make this more convenient. Maybe have basic C types implicitly defined in any lib type? Or put them in a |
There is an added complication here: |
I think that is just a technicality. The other standard types are the integer and float type for which we have aliases in My idea would be to put these standard C types in a namespace called |
Will the compiler disallow things other than alias definitions in |
Probably not, why? I mean it should be discouraged to use it for anything else. But I don't think the compiler needs to enforce it. |
Because most likely we'd start with normal |
Ah yes. That should happen when including However, maybe |
I am also worried that Perhaps including |
Also the platform-specific directory selection in |
I suppose so. There should be no issue with that except that it's in the namespace |
I think as a first step we should put those types in a new module # src/c_types.cr
module CTypes
alias Int = Int32
end
# src/lib_c.cr
require "c_types"
module LibC
alias Int = CTypes::Int
end Being able to |
Is there really a problem to fix? What's wrong with I understand that the bindings are incomplete, that it can break if we're removing symbols the stdlib doesn't need anymore, but
Yes, drop the LibXYZ libs and put every extern C libs into LibC directly! No more artificial LibXML2 or LibPCRE that push us to alias every single symbol when we can use the actual symbol names, no need to alias C types: they're already available. |
The C runtime may not be present even though the C language and calling conventions etc. are there, e.g. when writing pure Win32 code and needing to avoid linking against But another advantage of
That would mean passing all the linker flags from every single lib as soon as the libs are required, even when the source code doesn't call any of those lib functions, because under normal circumstances you can't avoid calling into the C runtime. |
I agree 100% with @HertzDevil that separatation into individual I think it makes a lot of sense to define the basic C data types outside of |
I think there is a confusion around the extern C world: IMO
We don't want arch-dependent sizes for binary protocols (different targets would fail to communicate). Even ELF files use explicit integer sizes for example 😁
Good point. Still, that's only really a problem for PCRE in practice, because it's always required by the prelude. The GC is always used & linked, and other bindings (LLVM, OpenSSL, XML2, ...) must be explicitly required, in which case I don't see a problem to link against the library; it won't get linked if unused anyway.
This ain't backward compatible: we can't reopen
I think that's a problem: the types are target dependent but the docs will declare them as being an alias to a specific type, that depends on which target was passed to the docs generator. This can be confusing, if not misleading. The C type names could instead be manually documented in the guide about extern C bindings, with actual examples (not hardcoded aliases), or maybe even the list per target; that would be more useful. |
PCRE may be the only practical application currently in the core library. But we have an entire ecosystem to take into account. It's not that uncommon for libraries to define C bindings which may or may not be actually linked, depending on which parts of the library are used. Due to the way the Crystal compiler autogenerates linker flags, this would mean the linker may need libraries to be available, despite not being used. It's hard to gauge the effect of this, but I'd certainly expect that changing this would introduce a number of breakages. So we wouldn't be able to implement it easily. And I really don't understand why merging all C bindings into a single lib type should even be considered a good idea. Sure, in C there are no such namespaces or a notion about which library defines a symbol. But I think it's a great enhancement that Crystal has a means to provide more structure.
I don't think there's anything preventing us from using the actual symbol names. It's just conventional to drop the lib namespace prefixes, so that the Crystal name for Putting individual libraries' bindings into separate namespaces is also just a convention, but it has some practical consequences for generating linker arguments. |
I prefer using actual symbols because it's the actual name and we should always properly name things. Creating fun aliases or renaming struct names, we create a virtual interface, which requires one more hoop to go from Crystal to C and vice versa. For example when reading the shard's code, you must reach the lib definition to know the actual symbol to start searching its original documentation. Same when trying to understand a segfault stacktrace, or why the linker fails with missing symbols. It's one more hoop, but a boring one, then repeat with how many symbols you want to reach 😡 In the end we spend time on an abstraction that nobody will use in the end, and that only makes things harder to follow & decipher 😕 I prefer to cram everything together because this is the reality: everything's crammed together in C, and that avoids to repeat the same aliases to C types in each and every lib definition, without any protection, because I can create a new LibFoo::SizeT that aliases UInt64 and that would work... until you compile for ARM32 and get segfaults. Though that's only for types and constants, because Cramming everything together avoids the above pitfalls, and we don't document lib definitions (on purpose: they're not meant to be used), so we don't care that everything's crammed together. Now it's under par because of the linker annotations, but maybe we could think differently? For example instead of merging the annotations each time we reopen Or maybe we don't need to do anything, or each lib could always include the C types by default, we could recommend to always use the actual symbols (don't waste your time on that intermediate layer, better: autogenerate it). |
The bindings in
LibC
are incomplete and not intended as public API exposed in the standard library. They only go as far as needed in terms of building blocks for stlib implementations.Moving the bindings into
Crystal::System
namespace explicitly communicates that they are an internal tool and not a public API.This has been mentioned before in #7339 (comment) and #11955 (comment)
It's to be expected that
LibC
bindings from stdlib are called from user code. It's not intended for that, but this was not explicitly communicated in the past.LibC
is not mentioned in the API docs because the code generator doesn't produce docs for lib types. So that could be understood as a technical limitation, not a intentional choice.I think it would be too hard to entirely remove
LibC
from stdlib and break existing code. I'd rather keep such a breaking change for the next major release, even though we might not critically bound to it. However, there's also not a huge cost for keeping the status quo around.The easiest implementation to retain the API would probably be
alias LibC = Crystal::System::LibC
.We could also consider duplicating the bindings so we can move independently, but I fear this would be too much overhead and complexity.
The text was updated successfully, but these errors were encountered: