[Ironruby-core] Code Review: socket2

Peter Bacon Darwin bacondarwin at googlemail.com
Tue May 6 13:20:00 EDT 2008

Hi John,
What is the process of these code reviews of external contributions?  Are
you expecting to begin a dialogue, since you do ask questions throughout the
review?  Or is it more of a feedback opportunity to let us know what you
have done to the code?  For instance, are you expecting me to make changes
discussed below and resubmit the patch - clearly not in the case of the
first item as you have said that you changed it already on your local copy?

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org
[mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of John Lam
Sent: Monday,05 May 05, 2008 18:07
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: [Ironruby-core] Code Review: socket2

tfpt review /shelveset:socket2;REDMOND\jflam

Overall - looks good! We really want to turn on tests for this stuff. Jim's
looking at getting the latest rubinius specs running so we can deprecate our
current spec snapshot soon - hopefully this week.

A few minor nits below:

Seems like you have a couple of redundant Protocols.CastToString calls here
- domain is *always* a MutableString based on your ctor ...

        private static Socket CreateSocket(CodeContext/*!*/ context,
MutableString/*!*/ domain, MutableString/*!*/ type, object/*Numeric*/
protocol) {
            RubyExecutionContext ec =
            RubyClass rubySocketClass =

            AddressFamily addressFamily =
(AddressFamily)RubyUtils.GetConstant(context, rubySocketClass,
SymbolTable.StringToId(Protocols.CastToString(context, domain)), true);
            ProtocolType protocolType =
(ProtocolType)(Protocols.CastToFixnum(context, protocol));
            SocketType socketType =
(SocketType)RubyUtils.GetConstant(context, rubySocketClass,
SymbolTable.StringToId(Protocols.CastToString(context, type)), true);
            return new Socket(addressFamily, socketType, protocolType);

Also ... Socket should be Socket/*!*/ since there is only one code path out
of this method barring exceptions. I've fixed these minor things in my copy.

Following this analysis, this also leads to _socket in RubyBasicSocket:23
being a /*!*/ as well.

BTW, I really like the /*Numeric*/ annotations that you've been putting in
your code. This could lead us to doing something smarter in the future in
the binder like adding a parameter attribute [Numeric] to coerce the binder
into doing some smarter things with the conversions.

Thinking aloud about this method - would it make more sense to have
MutableString and int overloads that delegate to helpers rather than
dropping everything into a single method like this one here?

        internal static MutableString ConvertToHostString(CodeContext/*!*/
context, object hostname) {
            if (hostname == null) {
                return null;
            if (hostname is MutableString) {
                MutableString strHostname = (MutableString)hostname;
                // Special cases
                if (strHostname == "" ) {
                    strHostname = new MutableString("");
                } else if (strHostname == "<broadcast>") {
                    strHostname = new MutableString("");
                return strHostname;
            int iHostname;
            if (Protocols.IntegerAsFixnum(hostname, out iHostname)) {
                // Ruby uses Little Endian whereas .NET uses Big Endian IP
                byte[] bytes = new byte[4];
                for (int i = 3; i >= 0; --i) {
                    bytes[i] = (byte)(iHostname & 0xff);
                    iHostname >>= 8;
                return new MutableString(new
            return Protocols.CastToString(context, hostname);

Some other points - seems some of the code below could be delegated to the
Protocols.CheckSafeLevel() helpers that you added. BTW, we should probably
spend some time and make a call about what we're going to do about all of
the safe level stuff. Arguably this stuff isn't necessary due to CLR safety
(especially in Silverlight contexts).

        internal static void CheckSecurity(CodeContext/*!*/ context, object
self, string message) {
            RubyExecutionContext ec =
            if (ec.CurrentSafeLevel >= 4 && ec.IsObjectTainted(self)) {
                throw RubyExceptions.CreateSecurityError("Insecure: " +


More information about the Ironruby-core mailing list