Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Undefined ivars return nil by default #7918

Closed
wants to merge 1 commit into from

4 participants

@ayrton

When calling an undefined ivar, it returns nil by default, however it does not create the ivar, thus there shouldn't be a performance penalty by this change, however it does improve readability imo.

if defined?(@expires_in) && defined?(@created_at) && @expires_in && @created_at

vs.

if @expires_in && @created_at`

What do you think?

Ran activesupport/test/caching_test.rb

@ayrton ayrton Undefined ivars return nil by default
When called and undefined ivar, it does not create the ivar and
set it to nil, so there should not be a performance penalty
1d29915
@josevalim
Owner

We need the defined checks otherwise we get Ruby warnings.

@josevalim
Owner

Although the proper solution here would be to initialize those variables in the initialize method, instead of having defined? checks creeping all over the source code.

@exviva

I think these checks were needed because new instance variables have been added and there still exist cached instances of that class, which will not have these variables.

I guess initializing these variables in initialize will not work, since Marshal.load does not call initialize (it calls marshal_load, it that helps). I'm not actually sure whether or not marshalling is used as serialization mechanism.

@fxn
Owner

The purpose is not to create uneeded ivars, because ivars take space in the serialized string and we want this to be extra efficient at the cost of some little unusual code, in order to tax the least small values like counters or flags.

@fxn
Owner

I believe this one can be closed, we do not want warnings, and we want to create strictly the ivars we need for space efficiency (see discussion in -core). If this rationale is not clear enough in source code comments a patch to the comments could be discussed.

@fxn fxn closed this
@ayrton

@fxn Not causing warnings is definitely a valid reason to not pull this in, I wasn't aware of this causing any warnings though. I didn't run Ruby in verbose mode when I was creating this commit. However after reading http://mislav.uniqpath.com/2011/06/ruby-verbose-mode/ it made more sense, perhaps we should mention the article in the contributing guides. Others might find this useful as well.

@fxn
Owner

@ayrton yeah, no worries I am sure you didn't propose a patch that issued warnings knowing that it did.

On the other hand, test suites run with warnings enabled, and one would see in this case

.../cache.rb:647: warning: instance variable @expires_in not initialized

Warnings are already covered in the contributing guide, and one is assumed to run the suite before submitting a pull request, I believe that is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 12, 2012
  1. @ayrton

    Undefined ivars return nil by default

    ayrton authored
    When called and undefined ivar, it does not create the ivar and
    set it to nil, so there should not be a performance penalty
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 10 deletions.
  1. +10 −10 activesupport/lib/active_support/cache.rb
View
20 activesupport/lib/active_support/cache.rb
@@ -558,15 +558,15 @@ def initialize(value, options = {})
end
def value
- convert_version_3_entry! if defined?(@value)
+ convert_version_3_entry! if @value
compressed? ? uncompress(@v) : @v
end
# Check if the entry is expired. The +expires_in+ parameter can override
# the value set when the entry was created.
def expired?
- convert_version_3_entry! if defined?(@value)
- if defined?(@x)
+ convert_version_3_entry! if @value
+ if @x
@x && @x < Time.now.to_i
else
false
@@ -574,7 +574,7 @@ def expired?
end
def expires_at
- Time.at(@x) if defined?(@x)
+ Time.at(@x) if @x
end
def expires_at=(value)
@@ -584,7 +584,7 @@ def expires_at=(value)
# Returns the size of the cached value. This could be less than
# <tt>value.size</tt> if the data is compressed.
def size
- if defined?(@s)
+ if @s
@s
else
case value
@@ -601,7 +601,7 @@ def size
# Duplicate the value in a class. This is used by cache implementations that don't natively
# serialize entries to protect against accidental cache modifications.
def dup_value!
- convert_version_3_entry! if defined?(@value)
+ convert_version_3_entry! if @value
if @v && !compressed? && !(@v.is_a?(Numeric) || @v == true || @v == false)
if @v.is_a?(String)
@v = @v.dup
@@ -622,7 +622,7 @@ def should_compress?(value, options)
end
def compressed?
- defined?(@c) ? @c : false
+ @c ? @c : false
end
def compress(value)
@@ -636,15 +636,15 @@ def uncompress(value)
# The internals of this method changed between Rails 3.x and 4.0. This method provides the glue
# to ensure that cache entries created under the old version still work with the new class definition.
def convert_version_3_entry!
- if defined?(@value)
+ if @value
@v = @value
remove_instance_variable(:@value)
end
- if defined?(@compressed)
+ if @compressed
@c = @compressed
remove_instance_variable(:@compressed)
end
- if defined?(@expires_in) && defined?(@created_at) && @expires_in && @created_at
+ if @expires_in && @created_at
@x = (@created_at + @expires_in).to_i
remove_instance_variable(:@created_at)
remove_instance_variable(:@expires_in)
Something went wrong with that request. Please try again.