Undefined ivars return nil by default #7918

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

ayrton commented Oct 12, 2012

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
Member

josevalim commented Oct 12, 2012

We need the defined checks otherwise we get Ruby warnings.

Member

josevalim commented Oct 12, 2012

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.

Contributor

exviva commented Oct 12, 2012

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.

Owner

fxn commented Oct 12, 2012

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.

Owner

fxn commented Oct 12, 2012

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 closed this Oct 12, 2012

Contributor

ayrton commented Oct 12, 2012

@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.

Owner

fxn commented Oct 12, 2012

@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