More PR feedback

Author: Paul van Brenk

Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs

@@ -8,8 +8,12 @@ namespace Microsoft.VisualStudioTools.Project
 {
     internal sealed class HierarchyIdMap
     {
-        private readonly ConcurrentDictionary<int, WeakReference<HierarchyNode>> nodes = new ConcurrentDictionary<int, WeakReference<HierarchyNode>>();
-        private readonly ConcurrentStack<int> freedIds = new ConcurrentStack<int>();
+        private readonly ConcurrentDictionary<uint, WeakReference<HierarchyNode>> nodes = new ConcurrentDictionary<uint, WeakReference<HierarchyNode>>();
+        private readonly ConcurrentStack<uint> freedIds = new ConcurrentStack<uint>();
+
+        public readonly static HierarchyIdMap Instance = new HierarchyIdMap();
+
+        private HierarchyIdMap() { }
 
         /// <summary>
         /// Must be called from the UI thread
@@ -33,15 +37,21 @@ public uint Add(HierarchyNode node)
 #endif
             if (!this.freedIds.TryPop(out var idx))
             {
-                idx = this.nodes.Count;
+                idx = this.NextIndex();
             }
 
             var addSuccess = this.nodes.TryAdd(idx, new WeakReference<HierarchyNode>(node));
             Debug.Assert(addSuccess, "Failed to add a new item");
 
-            return (uint)idx + 1;
+            return idx;
+        }
+
+        private uint NextIndex()
+        {
+            return (uint)this.nodes.Count + 1;
         }
 
+
         /// <summary>
         /// Must be called from the UI thread
         /// </summary>
@@ -51,7 +61,7 @@ public void Remove(HierarchyNode node)
 
             Debug.Assert(node != null, "Called with null node");
 
-            var idx = (int)node.ID - 1;
+            var idx = node.ID;
 
             var removeCheck = this.nodes.TryRemove(idx, out var weakRef);
 
@@ -71,14 +81,11 @@ public HierarchyNode this[uint itemId]
             {
                 Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess());
 
-                var idx = (int)itemId - 1;
-                if (0 <= idx && idx < this.nodes.Count)
+                var idx = itemId;
+                if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node))
                 {
-                    if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node))
-                    {
-                        Debug.Assert(node != null);
-                        return node;
-                    }
+                    Debug.Assert(node != null);
+                    return node;
                 }
 
                 // This is a valid return value, this gets called by VS after we deleted the item

Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs

@@ -158,9 +158,6 @@ internal enum EventTriggering
         /// <summary>A project will only try to build if it can obtain a lock on this object</summary>
         private volatile static object BuildLock = new object();
 
-        /// <summary>Maps integer ids to project item instances</summary>
-        private HierarchyIdMap itemIdMap = new HierarchyIdMap();
-
         /// <summary>A service provider call back object provided by the IDE hosting the project manager</summary>
         private IServiceProvider site;
 
@@ -590,7 +587,7 @@ public virtual string OutputBaseRelativePath
         /// <summary>
         /// Gets a collection of integer ids that maps to project item instances
         /// </summary>
-        internal HierarchyIdMap ItemIdMap => this.itemIdMap;
+        internal HierarchyIdMap ItemIdMap => HierarchyIdMap.Instance;
 
         /// <summary>
         /// Get the helper object that track document changes.