Skip to content

Commit

Permalink
Changed some uses of MutableString.Empty (like Array#join with empty …
Browse files Browse the repository at this point in the history
…array) to create a new empty string as the user could validly mutate it.

- Renamed MutableString.Empty to MutableString.FixedEmpty to make it clear that the instance should be shared in limited scenarios
Fixes to Module#instance_method per Tomas's feedback from previous code review
File.extname(".foo") should return "", not ".foo"
Removes to_proc hack from Libs\hacks.rb
  • Loading branch information
Shri Borde committed May 12, 2009
1 parent faa01a1 commit 94a7a95
Show file tree
Hide file tree
Showing 22 changed files with 132 additions and 55 deletions.

This file was deleted.

@@ -1,2 +1,2 @@
fails:File.split splits the given string into a directory and a file component and returns them in a two-element array. (edge cases)
fails:File.split splits the given string into a directory and a file component and returns them in a two-element array. (windows)
fails:File.split deals with multiple forward slashes
fails:File.split deals with Windows edge cases
@@ -0,0 +1 @@
fails:Module#instance_method returns an UnboundMethod corresponding to the given name from a singleton's superclass, but constrains it to the superclass, not the singleton class (for Class singletons)
Expand Up @@ -11,7 +11,7 @@
File.extname(".app.conf").should == ".conf"
end

it "returns the extension (the portion of file name in path after the period).(edge cases)" do
it "returns the extension (edge cases)" do
File.extname("").should == ""
File.extname(".").should == ""
File.extname("/").should == ""
Expand Down
Expand Up @@ -17,22 +17,39 @@

it "splits the given string into a directory and a file component and returns them in a two-element array. (edge cases)" do
File.split("").should == [".", ""]
File.split("//foo////").should == ["/", "foo"]
end

it "splits the given string into a directory and a file component and returns them in a two-element array. (windows)" do
compliant_on :ruby, :rubinius do
File.split(@path_windows_backward).should == [".", "C:\\foo\\bar\\baz.rb"]
platform_is_not :windows do
it "deals with multiple forward slashes" do
File.split("//foo////").should == ["/", "foo"]
end
end

platform_is :windows do
it "deals with multiple forward slashes" do
File.split("//foo////").should == ["//foo", "/"]
end

# Actually, MRI on windows behaves like this too, and that seems
# to be most proper behavior:
compliant_on :jruby do
File.split(@path_windows_backward).should == ["C:\\foo\\bar", "baz.rb"]
not_compliant_on :rubinius do
it "splits the given string into a directory and a file component and returns them in a two-element array. (windows)" do
File.split(@path_windows_backward).should == ["C:\\foo\\bar", "baz.rb"]
end
end

# Note: MRI on Cygwin exhibits third type of behavior,
# different from *both* variants above...
deviates_on :rubinius do
# Note: MRI on Cygwin exhibits third type of behavior,
# different from *both* variants above...
it "splits the given string into a directory and a file component and returns them in a two-element array. (windows)" do
File.split(@path_windows_backward).should == [".", "C:\\foo\\bar\\baz.rb"]
end
end

it "deals with Windows edge cases" do
File.split("c:foo").should == ["c:.", "foo"]
File.split("c:.").should == ["c:.", "."]
File.split("c:/foo").should == ["c:/", "foo"]
File.split("c:/.").should == ["c:/", "."]
end
end

it "splits the given string into a directory and a file component and returns them in a two-element array. (forward slash)" do
Expand Down
Expand Up @@ -252,9 +252,19 @@ def child_foo() :child_foo; end

InstanceMethodSingletonClass = class << InstanceMethodSingleton
def singleton_method() :singleton_method; end
self
self
end

SingletonInstanceMeth = class << InstanceMeth
def singleton_class_method() :singleton_class_method end
self
end

SingletonInstanceMethChild = class << InstanceMethChild
def singleton_subclass_method() :singleton_subclass_method end
self
end

module ClassVars
class A
@@a_cvar = :a_cvar
Expand Down Expand Up @@ -299,6 +309,17 @@ class SubModule < Module
def initialize
@special = 10
end

def module_method() :module_method end
end

SubModuleInstance = SubModule.new

SubModuleSingleton = SubModule.new

SubModuleSingletonClass = class << SubModuleSingleton
def module_singleton_method() :module_singleton_method end
self
end

module MA; end
Expand Down
Expand Up @@ -7,8 +7,17 @@
@superclass_um = ModuleSpecs::InstanceMethChild.instance_method(:foo)
@child_um = ModuleSpecs::InstanceMethChild.instance_method(:child_foo)
@mod_um = ModuleSpecs::InstanceMethChild.instance_method(:module_method)

# Singletons behave a bit differently
@singleton_um = ModuleSpecs::InstanceMethodSingletonClass.instance_method(:singleton_method)
@singleton_superclass_um = ModuleSpecs::InstanceMethodSingletonClass.instance_method(:foo)

@module_singleton_um = ModuleSpecs::SubModuleSingletonClass.instance_method(:module_singleton_method)
@module_singleton_superclass_um = ModuleSpecs::SubModuleSingletonClass.instance_method(:name)

@singleton_class_singleton_um = ModuleSpecs::SingletonInstanceMethChild.instance_method(:singleton_subclass_method)
@singleton_class_singleton_superclass_um = ModuleSpecs::SingletonInstanceMethChild.instance_method(:singleton_class_method)

end

it "returns an UnboundMethod" do
Expand Down Expand Up @@ -40,12 +49,24 @@

it "returns an UnboundMethod corresponding to the given name from a singleton class" do
@singleton_um.bind(ModuleSpecs::InstanceMethodSingleton).call.should == :singleton_method
@module_singleton_um.bind(ModuleSpecs::SubModuleSingleton).call.should == :module_singleton_method
@singleton_class_singleton_um.bind(ModuleSpecs::InstanceMethChild).call.should == :singleton_subclass_method
end

it "returns an UnboundMethod corresponding to the given name from a singleton's superclass, but constrains it to the superclass, not the singleton class" do
lambda { @singleton_superclass_um.bind(ModuleSpecs::InstanceMeth.new) }.should raise_error(TypeError)
@singleton_superclass_um.bind(ModuleSpecs::InstanceMethChild.new).call.should == :foo
@singleton_superclass_um.bind(ModuleSpecs::InstanceMethodSingleton).call.should == :foo

lambda { @module_singleton_superclass_um.bind(Module.new) }.should raise_error(TypeError)
@module_singleton_superclass_um.bind(ModuleSpecs::SubModuleInstance).call.should == "ModuleSpecs::SubModuleInstance"
@module_singleton_superclass_um.bind(ModuleSpecs::SubModuleSingleton).call.should == "ModuleSpecs::SubModuleSingleton"
end

it "returns an UnboundMethod corresponding to the given name from a singleton's superclass, but constrains it to the superclass, not the singleton class (for Class singletons)" do
@singleton_class_singleton_superclass_um.bind(ModuleSpecs::InstanceMeth).call.should == :singleton_class_method
# Is this a Ruby bug? This behavior seems inconsistent with kind_of? since "ModuleSpecs::SingletonInstanceMethChild.kind_of? ModuleSpecs::SingletonInstanceMeth" returns true
lambda { @singleton_class_singleton_superclass_um.bind(ModuleSpecs::InstanceMethChild) }.should raise_error(TypeError)
end

it "gives UnboundMethod method name, Module defined in and Module extracted from" do
Expand All @@ -68,6 +89,18 @@

@singleton_superclass_um.inspect.should =~ /\bfoo\b/
@singleton_superclass_um.inspect.should =~ /\bModuleSpecs::InstanceMethChild\b/

@module_singleton_um.inspect.should =~ /\bmodule_singleton_method\b/
@module_singleton_um.inspect.should =~ /\bModuleSpecs::SubModuleSingleton\b/

@module_singleton_superclass_um.inspect.should =~ /\bname\b/
@module_singleton_superclass_um.inspect.should =~ /\bModule\b/

@singleton_class_singleton_um.inspect.should =~ /\bsingleton_subclass_method\b/
@singleton_class_singleton_um.inspect.should =~ /\bModuleSpecs::InstanceMethChild\b/

@singleton_class_singleton_superclass_um.inspect.should =~ /\bsingleton_class_method\b/
@singleton_class_singleton_superclass_um.inspect.should =~ /\bModuleSpecs::InstanceMeth\b/
end

it "raises a TypeError if the given name is not a string/symbol" do
Expand Down
Expand Up @@ -410,7 +410,14 @@ public static class Constants {

[RubyMethod("extname", RubyMethodAttributes.PublicSingleton)]
public static MutableString/*!*/ GetExtension(RubyClass/*!*/ self, [DefaultProtocol, NotNull]MutableString/*!*/ path) {
return MutableString.Create(Path.GetExtension(path.ConvertToString())).TaintBy(path);
string pathStr = path.ConvertToString();
string extension = Path.GetExtension(pathStr);
string filename = Path.GetFileName(pathStr);
if (extension == filename) {
// File.extname(".foo") should be "", but Path.GetExtension(".foo") returns ".foo"
extension = String.Empty;
}
return MutableString.Create(extension).TaintBy(path);
}

[RubyMethod("file?", RubyMethodAttributes.PublicSingleton)]
Expand Down Expand Up @@ -458,7 +465,7 @@ public static class Constants {
var list = part as IList;
if (list != null) {
if (list.Count == 0) {
str = MutableString.Empty;
str = MutableString.FrozenEmpty;
} else if (visitedLists != null && visitedLists.ContainsKey(list)) {
str = InfiniteRecursionMarker;
} else {
Expand Down
Expand Up @@ -857,8 +857,9 @@ public static class ModuleOps {

RubyModule constraint = self;
if (self.IsSingletonClass && method.DeclaringModule != self) {
constraint = ((RubyClass)self).NominalClass;
constraint = ((RubyClass)self).SuperClass;
}

// unbound method binable to any class with "constraint" mixin:
return new UnboundMethod(constraint, methodName, method);
}
Expand Down
Expand Up @@ -1125,7 +1125,7 @@ public class RangeParser {
[RubyMethod("each_line")]
public static object EachLine(BlockParam block, MutableString/*!*/ self, [DefaultProtocol]MutableString/*!*/ separator) {
if (separator == null) {
separator = MutableString.Empty;
separator = MutableString.CreateEmpty();
}

uint version = self.Version;
Expand Down Expand Up @@ -2117,7 +2117,7 @@ public class RangeParser {
[NotNull]RubyRegex/*!*/ regexp, [DefaultProtocol, Optional]int limit) {

if (regexp.IsEmpty) {
return Split(stringCast, scope, self, MutableString.Empty, limit);
return Split(stringCast, scope, self, MutableString.FrozenEmpty, limit);
}

if (self.IsEmpty) {
Expand Down
Expand Up @@ -92,18 +92,17 @@ public class UnboundMethod {
string/*!*/ classDisplayName) {

MutableString result = MutableString.CreateMutable();

result.Append("#<");
result.Append(classDisplayName);
result.Append(": ");

if (declaringModule.IsSingletonClass) {
result.Append(((RubyClass)declaringModule).SuperClass.GetName(context));
} else if (ReferenceEquals(targetModule, declaringModule)) {
result.Append(declaringModule.GetName(context));
if (ReferenceEquals(targetModule, declaringModule)) {
result.Append(declaringModule.GetDisplayName(context, true));
} else {
result.Append(targetModule.GetName(context));
result.Append(targetModule.GetDisplayName(context, true));
result.Append('(');
result.Append(declaringModule.GetName(context));
result.Append(declaringModule.GetDisplayName(context, true));
result.Append(')');
}

Expand Down
Expand Up @@ -1160,14 +1160,14 @@ public static class IListOps {
[RubyMethod("join")]
public static MutableString/*!*/ Join(ConversionStorage<MutableString>/*!*/ tosConversion, ConversionStorage<MutableString>/*!*/ tostrConversion, IList/*!*/ self, Object separator) {
if (self.Count == 0) {
return MutableString.Empty;
return MutableString.CreateEmpty();
}
return Join(tosConversion, self, separator != null ? Protocols.CastToString(tostrConversion, separator) : MutableString.Empty);
return Join(tosConversion, self, separator != null ? Protocols.CastToString(tostrConversion, separator) : MutableString.FrozenEmpty);
}

public static MutableString/*!*/ Join(ConversionStorage<MutableString>/*!*/ tosConversion, IList/*!*/ self, MutableString/*!*/ separator) {
MutableString result = MutableString.CreateMutable();
RecursiveJoin(tosConversion, self, separator ?? MutableString.Empty, result,
RecursiveJoin(tosConversion, self, separator ?? MutableString.FrozenEmpty, result,
new Dictionary<object, bool>(ReferenceEqualityComparer<object>.Instance)
);
return result;
Expand Down
Expand Up @@ -333,7 +333,7 @@ private sealed class AstVisitor : Walker {
var str = MakeNode(kind, parts.Count);

if (parts.Count == 1) {
str.Add(MutableString.Empty);
str.Add(MutableString.FrozenEmpty);
}

for (int i = 0; i < parts.Count; i++) {
Expand Down
Expand Up @@ -38,7 +38,7 @@ public sealed class StringScanner : RubyObject {

public StringScanner(RubyClass/*!*/ rubyClass)
: base(rubyClass) {
_scanString = MutableString.Empty;
_scanString = MutableString.FrozenEmpty;
}

#if !SILVERLIGHT
Expand Down
8 changes: 0 additions & 8 deletions Merlin/Main/Languages/Ruby/Libs/hacks.rb
Expand Up @@ -13,14 +13,6 @@
#
# ****************************************************************************

# Hacked up implementation of Method

class Method
def to_proc
Proc.new { |*args| self.call(*args) }
end
end

# Subclass Tracking

module SubclassTracking
Expand Down
2 changes: 1 addition & 1 deletion Merlin/Main/Languages/Ruby/Ruby/Builtins/MatchData.cs
Expand Up @@ -42,7 +42,7 @@ public partial class MatchData : IDuplicable {
}

public MatchData() {
_originalString = MutableString.Empty;
_originalString = MutableString.FrozenEmpty;
_match = Match.Empty;
}

Expand Down
8 changes: 7 additions & 1 deletion Merlin/Main/Languages/Ruby/Ruby/Builtins/MutableString.cs
Expand Up @@ -52,7 +52,9 @@ public partial class MutableString : IEquatable<MutableString>, IComparable<Muta
private const uint VersionMask = ~FlagsMask;
private const uint FrozenVersion = VersionMask;

public static readonly MutableString Empty = MutableString.Create(String.Empty).Freeze();
// The instance is frozen so that it can be shared, but it should not be used in places where
// it will be accessible from user code as the user code could try to mutate it.
public static readonly MutableString FrozenEmpty = CreateEmpty().Freeze();

#region Constructors

Expand Down Expand Up @@ -214,6 +216,10 @@ public MutableString()
return new MutableString(_encoding);
}

public static MutableString/*!*/ CreateEmpty() {
return MutableString.Create(String.Empty);
}

/// <summary>
/// Creates a copy of this instance, including content and taint.
/// Doesn't copy frozen state and instance variables.
Expand Down
10 changes: 0 additions & 10 deletions Merlin/Main/Languages/Ruby/Ruby/Builtins/RubyClass.cs
Expand Up @@ -198,16 +198,6 @@ public sealed partial class RubyClass : RubyModule, IDuplicable {
get { return _singletonClassOf; }
}

public RubyClass NominalClass {
get {
if (_isSingletonClass) {
return _superClass;
} else {
return this;
}
}
}

// A class defined in Ruby code (not libraries, CLR types)
public bool IsRubyClass {
get { return _isRubyClass; }
Expand Down
2 changes: 1 addition & 1 deletion Merlin/Main/Languages/Ruby/Ruby/Builtins/RubyModule.cs
Expand Up @@ -1763,7 +1763,7 @@ private enum ConstantLookupResult {
return result.Append('>', nestings);
} else if (_name == null) {
if (showEmptyName) {
return MutableString.Empty;
return MutableString.FrozenEmpty;
} else {
MutableString result = MutableString.CreateMutable();
result.Append("#<");
Expand Down
2 changes: 1 addition & 1 deletion Merlin/Main/Languages/Ruby/Ruby/Runtime/RubyOps.cs
Expand Up @@ -1382,7 +1382,7 @@ public static partial class RubyOps {
// Used for implicit conversions from System.Object to MutableString (to_s conversion like).
[Emitted]
public static MutableString/*!*/ ObjectToMutableString(object/*!*/ value) {
return (value != null) ? MutableString.Create(value.ToString(), RubyEncoding.UTF8) : MutableString.Empty;
return (value != null) ? MutableString.Create(value.ToString(), RubyEncoding.UTF8) : MutableString.FrozenEmpty;
}

[Emitted] // ProtocolConversionAction
Expand Down
6 changes: 6 additions & 0 deletions Merlin/Main/Languages/Ruby/Scripts/irtests.bat
Expand Up @@ -13,6 +13,8 @@ if "%1" == "-p" (

set IRTESTS_ERRORS=Results:

time /t > %TEMP%\irtests_start_time.log

:==============================================================================
: Builds

Expand Down Expand Up @@ -100,6 +102,10 @@ if defined PARALLEL_IRTESTS (
:==============================================================================

if "%IRTESTS_ERRORS%"=="Results:" (
echo Start and end times:
more %TEMP%\irtests_start_time.log
time /t

if defined PARALLEL_IRTESTS (
echo All builds succeeded...
) else (
Expand Down

0 comments on commit 94a7a95

Please sign in to comment.