CR.Feedback

tweaked some of recordingobjectbinder implementation. (changeset 1270066)

Src/Compilers/Core/Source/Serialization/ConcurrentRecordingObjectBinder.cs

@@ -2,6 +2,7 @@
 
 using System;
 using System.Collections.Concurrent;
+using System.Diagnostics;
 using System.Reflection;
 
 namespace Roslyn.Utilities
@@ -9,7 +10,7 @@ namespace Roslyn.Utilities
     /// <summary>
     /// A binder that gathers type/reader mappings during object writing
     /// </summary>
-    internal class ConcurrentRecordingObjectBinder : RecordingObjectBinder
+    internal sealed class ConcurrentRecordingObjectBinder : RecordingObjectBinder
     {
         private readonly ConcurrentDictionary<TypeKey, Type> typeMap =
             new ConcurrentDictionary<TypeKey, Type>();
@@ -20,20 +21,23 @@ internal class ConcurrentRecordingObjectBinder : RecordingObjectBinder
         public override Type GetType(string assemblyName, string typeName)
         {
             Type type;
-            this.typeMap.TryGetValue(new TypeKey(assemblyName, typeName), out type);
+            if (!this.typeMap.TryGetValue(new TypeKey(assemblyName, typeName), out type))
+            {
+                Debug.Fail(assemblyName + "/" + typeName + " don't exist");
+            }
+
             return type;
         }
 
         public override Func<ObjectReader, object> GetReader(Type type)
         {
             Func<ObjectReader, object> reader;
-            this.readerMap.TryGetValue(type, out reader);
-            return reader;
-        }
+            if (!this.readerMap.TryGetValue(type, out reader))
+            {
+                Debug.Fail(type.ToString() + " reader doesn't exist");
+            }
 
-        private bool HasType(Type type)
-        {
-            return this.typeMap.ContainsKey(new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName));
+            return reader;
         }
 
         private bool HasConstructor(Type type)
@@ -45,11 +49,8 @@ public override void Record(Type type)
         {
             if (type != null)
             {
-                if (!HasType(type))
-                {
-                    var key = new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName);
-                    this.typeMap.TryAdd(key, type);
-                }
+                var key = new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName);
+                this.typeMap.TryAdd(key, type);
             }
         }
 
@@ -58,13 +59,20 @@ public override void Record(object instance)
             if (instance != null)
             {
                 var type = instance.GetType();
-                Record(type);
 
                 var readable = instance as IObjectReadable;
-                if (readable != null && !HasConstructor(type))
+                if (readable != null)
                 {
+                    if (HasConstructor(type))
+                    {
+                        Debug.Assert(this.typeMap.ContainsKey(new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName)));
+                        return;
+                    }
+
                     this.readerMap.TryAdd(type, readable.GetReader());
                 }
+
+                Record(type);
             }
         }
     }

Src/Compilers/Core/Source/Serialization/SimpleRecordingObjectBinder.cs

@@ -2,6 +2,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Reflection;
 
 namespace Roslyn.Utilities
@@ -9,7 +10,7 @@ namespace Roslyn.Utilities
     /// <summary>
     /// A binder that gathers type/reader mappings during object writing
     /// </summary>
-    internal class SimpleRecordingObjectBinder : RecordingObjectBinder
+    internal sealed class SimpleRecordingObjectBinder : RecordingObjectBinder
     {
         private readonly Dictionary<TypeKey, Type> typeMap =
             new Dictionary<TypeKey, Type>();
@@ -20,20 +21,23 @@ internal class SimpleRecordingObjectBinder : RecordingObjectBinder
         public override Type GetType(string assemblyName, string typeName)
         {
             Type type;
-            this.typeMap.TryGetValue(new TypeKey(assemblyName, typeName), out type);
+            if (!this.typeMap.TryGetValue(new TypeKey(assemblyName, typeName), out type))
+            {
+                Debug.Fail(assemblyName + "/" + typeName + " don't exist");
+            }
+
             return type;
         }
 
         public override Func<ObjectReader, object> GetReader(Type type)
         {
             Func<ObjectReader, object> reader;
-            this.readerMap.TryGetValue(type, out reader);
-            return reader;
-        }
+            if (!this.readerMap.TryGetValue(type, out reader))
+            {
+                Debug.Fail(type.ToString() + " reader doesn't exist");
+            }
 
-        private bool HasType(Type type)
-        {
-            return this.typeMap.ContainsKey(new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName));
+            return reader;
         }
 
         private bool HasConstructor(Type type)
@@ -45,13 +49,10 @@ public override void Record(Type type)
         {
             if (type != null)
             {
-                if (!HasType(type))
+                var key = new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName);
+                if (!this.typeMap.ContainsKey(key))
                 {
-                    var key = new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName);
-                    if (!this.typeMap.ContainsKey(key))
-                    {
-                        this.typeMap.Add(key, type);
-                    }
+                    this.typeMap.Add(key, type);
                 }
             }
         }
@@ -61,16 +62,23 @@ public override void Record(object instance)
             if (instance != null)
             {
                 var type = instance.GetType();
-                Record(type);
 
                 var readable = instance as IObjectReadable;
-                if (readable != null && !HasConstructor(type))
+                if (readable != null)
                 {
+                    if (HasConstructor(type))
+                    {
+                        Debug.Assert(this.typeMap.ContainsKey(new TypeKey(type.GetTypeInfo().Assembly.FullName, type.FullName)));
+                        return;
+                    }
+
                     if (!this.readerMap.ContainsKey(type))
                     {
                         this.readerMap.Add(type, readable.GetReader());
                     }
                 }
+
+                Record(type);
             }
         }
     }