-
-
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
Allow Atomic
s of pointer types
#14401
Allow Atomic
s of pointer types
#14401
Conversation
Specs are (still) failing |
The failure happens when compiling require "spec"
require "xml"
describe Atomic do
describe "#compare_and_set" do
it "with pointer" do
atomic = Atomic.new(Pointer(Void).null)
atomic.get.should eq(Pointer(Void).null)
end
end
end
module XML
describe Reader do
describe "#to_unsafe" do
it "returns a pointer to the underlying LibXML::XMLTextReader" do
reader = Reader.new("<root/>")
reader.to_unsafe.should be_a(LibXML::XMLTextReader)
end
end
end
end |
Oh what a mess. What options do we have? I suppose we could skip either of those specs when building with an old compiler? 🤔 |
We could also simply replace the typedefs with aliases on versions before 1.3, or even without a version check |
Related discussion about dropping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exhibits the same issue as #14402: Pointer#address
and Pointer#object_id
are both UInt64, whatever the actual pointer size, so that will require a DWCAS on 32-bit archs :(
@ysbaddaden I suppose there's no way around that until we introduce arch-specific pointer sizes. If you need to put a pointer into an atomic, you'll have to eat that. This PR doesn't really change anything about that because the current workaround would be to put I suppose you could technically downcast the integer type on a arch with smaller pointer size, but unlikely anyone is going this extra length. Anyways, if you want, you can still do that after this PR. It just enables an easier use case without invalidating anything that worked before. |
Looking at this again, if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HertzDevil it looks perfect!
Shall we extract the libxml2 fix to have a distinct commit in master, or just squash it here?
I missed this comment. But it seems like a good idea, it's an unrelated change. |
This is needed for things like
Atomic(LibC::HANDLE)
(see #14389).