From 4e0b166950fad2d7025a1c5719e14a185946234a Mon Sep 17 00:00:00 2001 From: Ramsay Stirling II Date: Fri, 6 Jul 2018 17:48:58 +0100 Subject: [PATCH 1/2] Support keyword arguments MutableStruct, ImmutableStruct, and SettableStruct --- lib/concurrent/immutable_struct.rb | 8 +++--- lib/concurrent/mutable_struct.rb | 8 +++--- lib/concurrent/settable_struct.rb | 8 +++--- .../synchronization/abstract_struct.rb | 22 +++++++++++---- spec/concurrent/struct_shared.rb | 28 +++++++++++++++++-- 5 files changed, 53 insertions(+), 21 deletions(-) diff --git a/lib/concurrent/immutable_struct.rb b/lib/concurrent/immutable_struct.rb index 05b8035c0..a37eb18bc 100644 --- a/lib/concurrent/immutable_struct.rb +++ b/lib/concurrent/immutable_struct.rb @@ -71,20 +71,20 @@ def select(&block) end # @!macro struct_new - def self.new(*args, &block) + def self.new(*args, **kw_args, &block) clazz_name = nil if args.length == 0 raise ArgumentError.new('wrong number of arguments (0 for 1+)') elsif args.length > 0 && args.first.is_a?(String) clazz_name = args.shift end - FACTORY.define_struct(clazz_name, args, &block) + FACTORY.define_struct(clazz_name, args, kw_args, &block) end FACTORY = Class.new(Synchronization::LockableObject) do - def define_struct(name, members, &block) + def define_struct(name, members, kw_args, &block) synchronize do - Synchronization::AbstractStruct.define_struct_class(ImmutableStruct, Synchronization::Object, name, members, &block) + Synchronization::AbstractStruct.define_struct_class(ImmutableStruct, Synchronization::Object, name, members, kw_args, &block) end end end.new diff --git a/lib/concurrent/mutable_struct.rb b/lib/concurrent/mutable_struct.rb index 2a92d726a..d0c2ebebd 100644 --- a/lib/concurrent/mutable_struct.rb +++ b/lib/concurrent/mutable_struct.rb @@ -197,20 +197,20 @@ def []=(member, value) end # @!macro struct_new - def self.new(*args, &block) + def self.new(*args, **kw_args, &block) clazz_name = nil if args.length == 0 raise ArgumentError.new('wrong number of arguments (0 for 1+)') elsif args.length > 0 && args.first.is_a?(String) clazz_name = args.shift end - FACTORY.define_struct(clazz_name, args, &block) + FACTORY.define_struct(clazz_name, args, kw_args, &block) end FACTORY = Class.new(Synchronization::LockableObject) do - def define_struct(name, members, &block) + def define_struct(name, members, kw_args, &block) synchronize do - clazz = Synchronization::AbstractStruct.define_struct_class(MutableStruct, Synchronization::LockableObject, name, members, &block) + clazz = Synchronization::AbstractStruct.define_struct_class(MutableStruct, Synchronization::LockableObject, name, members, kw_args, &block) members.each_with_index do |member, index| clazz.send :remove_method, member clazz.send(:define_method, member) do diff --git a/lib/concurrent/settable_struct.rb b/lib/concurrent/settable_struct.rb index 9706cff2d..404726ba3 100644 --- a/lib/concurrent/settable_struct.rb +++ b/lib/concurrent/settable_struct.rb @@ -92,20 +92,20 @@ def []=(member, value) end # @!macro struct_new - def self.new(*args, &block) + def self.new(*args, **kw_args, &block) clazz_name = nil if args.length == 0 raise ArgumentError.new('wrong number of arguments (0 for 1+)') elsif args.length > 0 && args.first.is_a?(String) clazz_name = args.shift end - FACTORY.define_struct(clazz_name, args, &block) + FACTORY.define_struct(clazz_name, args, kw_args, &block) end FACTORY = Class.new(Synchronization::LockableObject) do - def define_struct(name, members, &block) + def define_struct(name, members, kw_args, &block) synchronize do - clazz = Synchronization::AbstractStruct.define_struct_class(SettableStruct, Synchronization::LockableObject, name, members, &block) + clazz = Synchronization::AbstractStruct.define_struct_class(SettableStruct, Synchronization::LockableObject, name, members, kw_args, &block) members.each_with_index do |member, index| clazz.send :remove_method, member if clazz.instance_methods.include? member clazz.send(:define_method, member) do diff --git a/lib/concurrent/synchronization/abstract_struct.rb b/lib/concurrent/synchronization/abstract_struct.rb index 868ffbf08..74164aa77 100644 --- a/lib/concurrent/synchronization/abstract_struct.rb +++ b/lib/concurrent/synchronization/abstract_struct.rb @@ -6,9 +6,18 @@ module Synchronization module AbstractStruct # @!visibility private - def initialize(*values) + def initialize(*values, **kw_values) super() - ns_initialize(*values) + ns_initialize(*values, **kw_values) + end + + # @!macro [attach] struct_keyword_init + # + # Returns `true` if the struct uses keyword arguments. + # + # @return [Boolean] true if struct uses keyword arguments + def keyword_init? + self.class::KEYWORD_INIT end # @!macro [attach] struct_length @@ -127,13 +136,14 @@ def pr_underscore(clazz) end # @!visibility private - def self.define_struct_class(parent, base, name, members, &block) + def self.define_struct_class(parent, base, name, members, kw_args, &block) clazz = Class.new(base || Object) do include parent self.const_set(:MEMBERS, members.collect{|member| member.to_s.to_sym}.freeze) - def ns_initialize(*values) - raise ArgumentError.new('struct size differs') if values.length > length - @values = values.fill(nil, values.length..length-1) + self.const_set(:KEYWORD_INIT, !!kw_args[:keyword_init]) + def ns_initialize(*values, **kw_values) + raise ArgumentError.new('struct size differs') if values.length > length || kw_values.length > length + @values = keyword_init? ? members.map{ |val| kw_values.fetch(val, nil) } : values.fill(nil, values.length..length-1) end end unless name.nil? diff --git a/spec/concurrent/struct_shared.rb b/spec/concurrent/struct_shared.rb index b67fd4c61..b88b8ceec 100644 --- a/spec/concurrent/struct_shared.rb +++ b/spec/concurrent/struct_shared.rb @@ -27,7 +27,8 @@ members = [:Foo, :bar, 'baz'] structs = [ described_class.new(*members).new, - described_class.new('ClassForCheckingGetterDefinition', *members).new + described_class.new(*members, keyword_init: true).new, + described_class.new('ClassForCheckingGetterDefinition', *members).new, ] structs.each do |struct| @@ -54,8 +55,11 @@ def baz(foo, bar) foo + bar; end clazz2 = described_class.new(:foo, :bar) do def baz(foo, bar) foo + bar; end end + clazz3 = described_class.new(:foo, :bar, keyword_init: true) do + def baz(foo, bar) foo + bar; end + end - [clazz1, clazz2].each do |clazz| + [clazz1, clazz2, clazz3].each do |clazz| struct = clazz.new expect(struct).to respond_to :baz expect(struct.method(:baz).arity).to eq 2 @@ -68,9 +72,11 @@ def baz(foo, bar) foo + bar; end let!(:members){ [:Foo, :bar, 'baz'] } let!(:values){ [42, '42', :fortytwo] } + let!(:kw_values){ {Foo: 42, bar: '42', baz: :fortytwo} } let!(:classes) do [ described_class.new(*members), + described_class.new(*members, keyword_init: true), described_class.new('StructConstructionTester', *members) ] end @@ -86,7 +92,7 @@ def baz(foo, bar) foo + bar; end it 'sets all given members in order' do classes.each do |clazz| - struct = clazz.new(*values) + struct = clazz::KEYWORD_INIT ? clazz.new(**kw_values) : clazz.new(*values) members.each_with_index do |member, index| expect(struct.send(member)).to eq values[index] end @@ -105,6 +111,7 @@ def baz(foo, bar) foo + bar; end let!(:anon_struct_members) { [:name, :address, :zip] } let(:anon_struct) { described_class.new(*anon_struct_members) } + let(:anon_struct_with_keyword_init) { described_class.new(*anon_struct_members, keyword_init: true) } let!(:named_struct_members) { [:left, :right] } let(:named_struct) do @@ -116,19 +123,31 @@ def baz(foo, bar) foo + bar; end it 'returns the number of struct members' do expect(anon_struct.new.length).to eq anon_struct_members.length + expect(anon_struct_with_keyword_init.new.length).to eq anon_struct_members.length expect(named_struct.new.length).to eq named_struct_members.length end end + context '#keyword_init?' do + + it 'returns a boolean indicating if keywords are enabled for initialization' do + expect(anon_struct.new.keyword_init?).to eq false + expect(anon_struct_with_keyword_init.new.keyword_init?).to eq true + expect(named_struct.new.keyword_init?).to eq false + end + end + context '#members' do it 'returns the struct members as an array of symbols' do expect(anon_struct.new.members).to eq anon_struct_members + expect(anon_struct_with_keyword_init.new.members).to eq anon_struct_members expect(named_struct.new.members).to eq named_struct_members end it 'returns a different object than the array passed at definition' do expect(anon_struct.new.members.object_id).to_not eq anon_struct_members.object_id + expect(anon_struct_with_keyword_init.new.members.object_id).to_not eq anon_struct_members.object_id expect(named_struct.new.members.object_id).to_not eq named_struct_members.object_id end end @@ -137,6 +156,7 @@ def baz(foo, bar) foo + bar; end it 'returns the number of struct members' do expect(anon_struct.new.size).to eq anon_struct_members.size + expect(anon_struct_with_keyword_init.new.size).to eq anon_struct_members.size expect(named_struct.new.size).to eq named_struct_members.size end end @@ -145,9 +165,11 @@ def baz(foo, bar) foo + bar; end it 'returns the values of the struct as an array in order' do expect(anon_struct.new().values).to eq [nil, nil, nil] + expect(anon_struct_with_keyword_init.new().values).to eq [nil, nil, nil] expect(named_struct.new().values).to eq [nil, nil] expect(anon_struct.new(:foo, :bar, :baz).values).to eq [:foo, :bar, :baz] + expect(anon_struct_with_keyword_init.new(name: :foo, address: :bar, zip: :baz).values).to eq [:foo, :bar, :baz] expect(named_struct.new(:yes, :no).values).to eq [:yes, :no] end end From 06f8a02678e282838e98b8edce048d0cb65431bb Mon Sep 17 00:00:00 2001 From: Ramsay Stirling II Date: Sat, 7 Jul 2018 01:14:34 +0100 Subject: [PATCH 2/2] Ensure keyword arguments for Concurrent::*Struct behave similarly to Std-lib Struct --- lib/concurrent/synchronization/abstract_struct.rb | 10 ++++++++-- spec/concurrent/struct_shared.rb | 9 +++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/concurrent/synchronization/abstract_struct.rb b/lib/concurrent/synchronization/abstract_struct.rb index 74164aa77..b54abe11d 100644 --- a/lib/concurrent/synchronization/abstract_struct.rb +++ b/lib/concurrent/synchronization/abstract_struct.rb @@ -142,8 +142,14 @@ def self.define_struct_class(parent, base, name, members, kw_args, &block) self.const_set(:MEMBERS, members.collect{|member| member.to_s.to_sym}.freeze) self.const_set(:KEYWORD_INIT, !!kw_args[:keyword_init]) def ns_initialize(*values, **kw_values) - raise ArgumentError.new('struct size differs') if values.length > length || kw_values.length > length - @values = keyword_init? ? members.map{ |val| kw_values.fetch(val, nil) } : values.fill(nil, values.length..length-1) + @values = if keyword_init? + key_diff = kw_values.keys - members + raise ArgumentError.new("unknown keywords: #{key_diff.join(',')}") unless key_diff.empty? + members.map {|val| kw_values.fetch(val, nil)} + else + raise ArgumentError.new('struct size differs') if values.length > length + values.fill(nil, values.length..length-1) + end end end unless name.nil? diff --git a/spec/concurrent/struct_shared.rb b/spec/concurrent/struct_shared.rb index b88b8ceec..77f8fe1ee 100644 --- a/spec/concurrent/struct_shared.rb +++ b/spec/concurrent/struct_shared.rb @@ -101,8 +101,13 @@ def baz(foo, bar) foo + bar; end it 'raises an exception when extra members are given' do classes.each do |clazz| - extra_values = values << 'forty two' - expect{ clazz.new(*extra_values) }.to raise_error(ArgumentError) + if clazz::KEYWORD_INIT + extra_values = kw_values.merge({FortyTwo: 'forty two'}) + expect{ clazz.new(**extra_values) }.to raise_error(ArgumentError) + else + extra_values = values << 'forty two' + expect{ clazz.new(*extra_values) }.to raise_error(ArgumentError) + end end end end