Skip to content
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

Code cleanups 3rd try #17

Closed
wants to merge 3 commits into from
Closed

Code cleanups 3rd try #17

wants to merge 3 commits into from

Conversation

qmx
Copy link
Member

@qmx qmx commented Dec 30, 2010

this time following wmeissner's performance tips

@enebo
Copy link
Member

enebo commented Dec 30, 2010

whoops...I should have put a comment when I closed this that I applied all three patches. The RubyModule one I did a minor tweak from getRuntime() to runtime since your new method passes runtime in.

@qmx
Copy link
Member Author

qmx commented Dec 30, 2010

looks like 4068b5f got lost :(

@enebo
Copy link
Member

enebo commented Dec 30, 2010

I will land this after lunch...very odd...I know I cherry-picked it. I was curious about the patch since you deprecated the method and then made the non-deprecated method call it. I figured I would see a deprecation warning during compilation (but didn't perhaps because I did not actually cherry pick it?)

@enebo
Copy link
Member

enebo commented Dec 30, 2010

Yeah that last patch needs to be changed. Otherwise we will have one more deprecation warning each time we compile JRuby.

  [apt] /Users/enebo/work/jruby/src/org/jruby/embed/variable/ClassVariable.java:177: warning: [deprecation] removeCvar(org.jruby.runtime.builtin.IRubyObject) in org.jruby.RubyModule has been deprecated
  [apt]         rubyClass.removeCvar(rubyName);

@qmx
Copy link
Member Author

qmx commented Dec 30, 2010

It was like this before: #16

wmeissner told me to use removecvar to spare the ThreadLocal call

@enebo
Copy link
Member

enebo commented Dec 30, 2010

I am not a huge fan of Java code calling @jruby_method bound methods directly since most of those methods usually do more than a Java caller would want (not in this case though).

In general we have a set of lower level methods which are meant to Java callers to use and we have a set of JRubyMethods which typically process IRubyObject arguments and do coercions. The bigger problem with removeCVar is it should have been 'public IRubyObject removeCvar(String name)'. So I think the patch should @deprecate removeCvar...create a new removeClassVariable(String name). Change removeCVar and remove_class_variable to both call removeClassVariable. Change ClassVariable.remove to call removeClassVariable also (notice the smell at that callsite? We construct a RubyString based on name to remove the variable).

@qmx
Copy link
Member Author

qmx commented Dec 30, 2010

I've did the modifications here

qmx@c11a215

eregon added a commit that referenced this pull request Sep 27, 2016
5dae43c Remove extra parens in method definition
bba1434 Simplify logic in numeric helpers
97ad3d1 Add missing require
3b4b81f Require formatters when referring to their constants
a7ebabf Setup the environment for all commands so they can be run directly
1b0fe0f Try to load "#{RUBY_ENGINE}.mspec"
8c2c37e require the full mspec environment only when about to load spec files
ef6e996 Support running from a single spec file
a8333b4 Add a few missing require
8622c59 Delay requiring rbconfig
253d206 Expand the path before giving it to Kernel.load
e9353b3 Compute once the value of host OS and fallback on RUBY_PLATFORM if RbConfig is not available
f86fda4 Remove support for patchlevel in version since it is no longer used since 2.1
b32353f Gracefully handle missing pp library
c29fc35 Prefer ||= to defined? for clarity
268ff4c Prefer Object.const_defined? to defined? for mruby
81c7a43 Remove old support for iconv
d1f3c0b Merge pull request #17 from ksss/mruby
7090340 mruby is not implemented `defined?`
fa3e9c7 mruby dosen't work to concat string literal

git-subtree-dir: spec/mspec
git-subtree-split: 5dae43c8604db4357340f9a0891906b48e639a49
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants