Merge pull request #1396 from Archytaus/attr-perf-fix

Performance improvement of methods generated by Module#attr

3 files changed, 195 insertions, 25 deletions

diff --git a/mrblib/class.rb b/mrblib/class.rb
index 4f268b6c8..6d624ee83 100644
--- a/mrblib/class.rb
+++ b/mrblib/class.rb
@@ -1,18 +1,24 @@
 class Module
   # 15.2.2.4.13
   def attr_reader(*names)
-    names.each{|name|
-      name2 = ('@'+name.to_s).intern
-      define_method(name){self.instance_variable_get(name2)}
-    }
+    names.each do |name|
+      name = name.to_s
+      raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?')  || name.include?('$')
+      
+      attr_name = '@'+name
+      define_method(name){self.instance_variable_get(attr_name)}
+    end
   end
   # 15.2.2.4.14
   def attr_writer(*names)
-    names.each{|name|
-      name2 = ('@'+name.to_s).intern
-      name = (name.to_s+"=").intern
-      define_method(name){|v|self.instance_variable_set(name2,v)}
-    }
+    names.each do |name|
+      name = name.to_s
+      raise(NameError, "#{name.inspect} is not allowed as an instance variable name") if name.include?('@') || name.include?('?')  || name.include?('$')
+      
+      attr_name = '@'+name
+      name = (name+"=").intern
+      define_method(name){|v|self.instance_variable_set(attr_name,v)}
+    end
   end
   # 15.2.2.4.12
   def attr_accessor(*names)
diff --git a/src/kernel.c b/src/kernel.c
index ae8d24710..f2c7f182f 100644
--- a/src/kernel.c
+++ b/src/kernel.c
@@ -549,15 +549,37 @@ obj_is_instance_of(mrb_state *mrb, mrb_value self)
 }
 
 static void
-check_iv_name(mrb_state *mrb, mrb_sym id)
+valid_iv_name(mrb_state *mrb, mrb_sym iv_name_id, const char* s, size_t len) 
+{
+  if (len < 2 || !(s[0] == '@' && s[1] != '@')) {
+    mrb_name_error(mrb, iv_name_id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, iv_name_id));
+  }
+}
+
+static void
+check_iv_name(mrb_state *mrb, mrb_sym iv_name_id)
 {
   const char *s;
   size_t len;
 
-  s = mrb_sym2name_len(mrb, id, &len);
-  if (len < 2 || !(s[0] == '@' && s[1] != '@')) {
-    mrb_name_error(mrb, id, "`%S' is not allowed as an instance variable name", mrb_sym2str(mrb, id));
+  s = mrb_sym2name_len(mrb, iv_name_id, &len);
+  valid_iv_name(mrb, iv_name_id, s, len);
+}
+
+static mrb_sym
+get_valid_iv_sym(mrb_state *mrb, mrb_value iv_name)
+{
+  mrb_sym iv_name_id;
+
+  if (mrb_string_p(iv_name)) {
+    iv_name_id = mrb_intern_cstr(mrb, RSTRING_PTR(iv_name));
+    valid_iv_name(mrb, iv_name_id, RSTRING_PTR(iv_name), RSTRING_LEN(iv_name));
+  } else if(mrb_symbol_p(iv_name)) {
+    iv_name_id = mrb_symbol(iv_name);
+    check_iv_name(mrb, iv_name_id);
   }
+
+  return iv_name_id;
 }
 
 /* 15.3.1.3.20 */
@@ -582,10 +604,11 @@ mrb_value
 mrb_obj_ivar_defined(mrb_state *mrb, mrb_value self)
 {
   mrb_sym mid;
+  mrb_value sym;
   mrb_bool defined_p;
 
-  mrb_get_args(mrb, "n", &mid);
-  check_iv_name(mrb, mid);
+  mrb_get_args(mrb, "o", &sym);
+  mid = get_valid_iv_sym(mrb, sym);
   defined_p = mrb_obj_iv_defined(mrb, mrb_obj_ptr(self), mid);
 
   return mrb_bool_value(defined_p);
@@ -614,12 +637,13 @@ mrb_obj_ivar_defined(mrb_state *mrb, mrb_value self)
 mrb_value
 mrb_obj_ivar_get(mrb_state *mrb, mrb_value self)
 {
-  mrb_sym id;
-
-  mrb_get_args(mrb, "n", &id);
+  mrb_sym iv_name_id;
+  mrb_value iv_name;
+  
+  mrb_get_args(mrb, "o", &iv_name);
 
-  check_iv_name(mrb, id);
-  return mrb_iv_get(mrb, self, id);
+  iv_name_id = get_valid_iv_sym(mrb, iv_name);
+  return mrb_iv_get(mrb, self, iv_name_id);
 }
 
 /* 15.3.1.3.22 */
@@ -645,12 +669,13 @@ mrb_obj_ivar_get(mrb_state *mrb, mrb_value self)
 mrb_value
 mrb_obj_ivar_set(mrb_state *mrb, mrb_value self)
 {
-  mrb_sym id;
-  mrb_value val;
+  mrb_sym iv_name_id;
+  mrb_value iv_name, val;
 
-  mrb_get_args(mrb, "no", &id, &val);
-  check_iv_name(mrb, id);
-  mrb_iv_set(mrb, self, id, val);
+  mrb_get_args(mrb, "oo", &iv_name, &val);
+  
+  iv_name_id = get_valid_iv_sym(mrb, iv_name);
+  mrb_iv_set(mrb, self, iv_name_id, val);
   return val;
 }
 
diff --git a/test/t/module.rb b/test/t/module.rb
index 9d735f5da..618b57f06 100644
--- a/test/t/module.rb
+++ b/test/t/module.rb
@@ -37,6 +37,145 @@ assert('Module#append_features', '15.2.2.4.10') do
   assert_equal Test4AppendFeatures2.const_get(:Const4AppendFeatures2), Test4AppendFeatures2
 end
 
+assert('Module#attr NameError') do
+  %w[
+    foo?
+    @foo
+    @@foo
+    $foo
+  ].each do |name|
+    module NameTest; end
+
+    assert_raise(NameError) do
+      NameTest.module_eval { attr_reader name.to_sym }
+    end
+
+    assert_raise(NameError) do
+      NameTest.module_eval { attr_writer name.to_sym }
+    end
+
+    assert_raise(NameError) do
+      NameTest.module_eval { attr name.to_sym }
+    end
+
+    assert_raise(NameError) do
+      NameTest.module_eval { attr_accessor name.to_sym }
+    end
+  end
+
+end
+
+assert('Module#attr', '15.2.2.4.11') do
+  class AttrTest
+    class << self
+      attr :cattr
+      def cattr_val=(val)
+        @cattr = val
+      end
+    end
+    attr :iattr
+    def iattr_val=(val)
+      @iattr = val
+    end
+  end
+
+  test = AttrTest.new
+  assert_true AttrTest.respond_to?(:cattr)
+  assert_true test.respond_to?(:iattr)
+
+  assert_false AttrTest.respond_to?(:vattr=)
+  assert_false test.respond_to?(:iattr=)
+
+  test.iattr_val = 'test'
+  assert_equal 'test', test.iattr
+
+  AttrTest.cattr_val = 'test'
+  assert_equal 'test', AttrTest.cattr
+end
+
+assert('Module#attr_accessor', '15.2.2.4.12') do
+  class AttrTestAccessor
+    class << self
+      attr_accessor :cattr
+    end
+    attr_accessor :iattr, 'iattr2'
+  end
+
+  attr_instance = AttrTestAccessor.new
+  assert_true AttrTestAccessor.respond_to?(:cattr=)
+  assert_true attr_instance.respond_to?(:iattr=)
+  assert_true attr_instance.respond_to?(:iattr2=)
+  assert_true AttrTestAccessor.respond_to?(:cattr)
+  assert_true attr_instance.respond_to?(:iattr)
+  assert_true attr_instance.respond_to?(:iattr2)
+
+  attr_instance.iattr = 'test'
+  assert_equal 'test', attr_instance.iattr
+
+  AttrTestAccessor.cattr = 'test'
+  assert_equal 'test', AttrTestAccessor.cattr
+end
+
+assert('Module#attr_reader', '15.2.2.4.13') do
+  class AttrTestReader
+    class << self
+      attr_reader :cattr
+      def cattr_val=(val)
+        @cattr = val
+      end
+    end
+    attr_reader :iattr, 'iattr2'
+    def iattr_val=(val)
+      @iattr = val
+    end
+  end
+
+  attr_instance = AttrTestReader.new
+  assert_true AttrTestReader.respond_to?(:cattr)
+  assert_true attr_instance.respond_to?(:iattr)
+  assert_true attr_instance.respond_to?(:iattr2)
+
+  assert_false AttrTestReader.respond_to?(:cattr=)
+  assert_false attr_instance.respond_to?(:iattr=)
+  assert_false attr_instance.respond_to?(:iattr2=)
+
+  attr_instance.iattr_val = 'test'
+  assert_equal 'test', attr_instance.iattr
+
+  AttrTestReader.cattr_val = 'test'
+  assert_equal 'test', AttrTestReader.cattr
+end
+
+assert('Module#attr_writer', '15.2.2.4.14') do
+  class AttrTestWriter
+    class << self
+      attr_writer :cattr
+      def cattr_val
+        @cattr
+      end
+    end
+    attr_writer :iattr, 'iattr2'
+    def iattr_val
+      @iattr
+    end
+  end
+
+  attr_instance = AttrTestWriter.new
+  assert_true AttrTestWriter.respond_to?(:cattr=)
+  assert_true attr_instance.respond_to?(:iattr=)
+  assert_true attr_instance.respond_to?(:iattr2=)
+
+  assert_false AttrTestWriter.respond_to?(:cattr)
+  assert_false attr_instance.respond_to?(:iattr)
+  assert_false attr_instance.respond_to?(:iattr2)
+
+  attr_instance.iattr = 'test'
+  assert_equal 'test', attr_instance.iattr_val
+
+  AttrTestWriter.cattr = 'test'
+  assert_equal 'test', AttrTestWriter.cattr_val
+end
+
 assert('Module#class_eval', '15.2.2.4.15') do
   class Test4ClassEval
     @a = 11